| | <<<Back 1 day (to 2017/05/08) | 20170509 |
| avih | tor8: morning. i think there's use of uninitialized value at the line number when doing this: js_loadstring(J, "x"); (x is not defined) and then calling it. i'm expecting line 1 error, but i get the line number of the previously compiled file/string. | 09:37.17 |
| | number of the last line of the previously compiled file | 09:37.55 |
| | there are also some other cases where i'm getting line number smaller than i expect, when the erroneous thing follows some newlines or comments | 09:39.19 |
| | not sure if it's the same bug or not, but it could, as it seem on some cases it doesn't update the line number and just uses whatever it had there previously. | 09:40.24 |
| tor8 | avih: have you got an example file exhibiting the error? | 09:41.42 |
| avih | i just gave you | 09:41.51 |
| | just js_loadstring(J, "x"); and call it | 09:42.22 |
| tor8 | works for me. | 09:43.14 |
| avih | admittedly i haven't tested this minimal case, so i will just now | 09:43.18 |
| tor8 | a simple 5-line C file that exhibits the problem would go a long way | 09:43.34 |
| avih | indeed | 09:43.40 |
| tor8 | avih: pop in a "debugger" statement and you can see the stack and scope and bytecode for the function | 09:48.06 |
| | js_loadstring(J, "debugger;x") | 09:48.17 |
| avih | tor8: not as minimal as i hoped, but hopefully you can reproduce https://pastebin.mozilla.org/9021198 | 10:16.28 |
| | of course newcfunction should be with 2, but it doesn't matter here anyway. | 10:18.12 |
| | (inconsequential bug in my demo) | 10:18.34 |
| | tor8: can you reproduce? | 10:29.27 |
| | and confirm it's a bug? (two different things :) ) | 10:29.45 |
| | (well, there's no "x.js:1" so it must be a bug.. | 10:30.25 |
| | no x.js:4 | 10:30.37 |
| tor8 | avih: yes, I can reproduce what you're seeing | 10:35.33 |
| avih | (also, js_dostring should get stackTrace too) | 10:35.36 |
| tor8 | avih: in jsP_next (in jsparse.c), try moving J->astline = J->lexline to *after* the jsY_lex call | 10:50.46 |
| avih | huh, i actually thought i validated the lexing and parsing, and wasn't sure where it happens between compile and run. will try your sugggestion though | 10:51.47 |
| tor8 | it's tagging the first token with the "uninitialized" line number when building the AST | 10:52.14 |
| | I *think* the proposed fix should work, but I'm a bit fuzzy on the exact details | 10:52.53 |
| | we tag each AST node in the parse with a line number, then we copy that into the bytecode for each statement/expression where the line number changes | 10:53.31 |
| avih | heh | 10:53.31 |
| tor8 | it's been a while since I touched the parser... | 10:53.44 |
| avih | tor8: fixes the test case | 10:54.52 |
| | tor8: is the 'this' object used when calling a thing inserted with js_loadstring? e.g. i did pushglobal in my example, and iirc pushundefined works too, but what happens if it's something else? | 10:56.53 |
| | (_iirc_ it broke somehow) | 10:57.01 |
| tor8 | what happens when you dereference 'this' depends on the strictness mode | 10:58.19 |
| | the 'script' object created with js_loadstring needs to be called just like all other functions, with a this binding | 10:59.42 |
| | but it takes no arguments, and it does some extra voodoo to deal with scoping rules | 10:59.56 |
| avih | right, so what happens if i push an {x:7} object as its this? | 11:00.20 |
| tor8 | if you're not doing "foo.bar()" type calls, you *should* always be passing undefined as the this object | 11:00.20 |
| avih | i'm talking the this i push after js_loadstring | 11:00.55 |
| | is it used? | 11:01.11 |
| | line 25 in my example. how does the object pushed there affects anything? | 11:01.38 |
| tor8 | if you push {x:7}, then any 'this' expressions in the script will evaluate to that object | 11:01.42 |
| avih | gotcha | 11:01.52 |
| | so it sets the script's own global object to that? | 11:02.10 |
| tor8 | all that happens with the 'this' value is it's available to the caller if they use a 'this' expression | 11:02.21 |
| avih | effectively hides the real global object from that script? | 11:02.27 |
| tor8 | nope, nothing to do with global objects | 11:02.30 |
| | *however* ... in non-strict mode, if you try to use 'this' and the this binding is null or undefined, it gets the global object instead | 11:02.58 |
| | as per spec | 11:03.01 |
| avih | so, the default is not strict, so every js_loadstring is initially non strict, unless the source itself uses 'use strict'; | 11:03.20 |
| tor8 | correct. | 11:03.36 |
| | unless you pass JS_STRICT to js_newstate | 11:03.52 |
| avih | right, which i don't. so if i push {x:7} when calling after js_loadstring, can the loaded string access the global object at all? since regardless of strictness, 'this' at the top scope of the script is the global object, isn't it? | 11:05.15 |
| | top lexical scope*, i.e. the first line of the source | 11:06.02 |
| | e.g.* | 11:06.07 |
| tor8 | you can always access the properties in the global object, but the only way in JS to get at the object representing the global scope is via an unbound (null or undefined) this access | 11:06.08 |
| | and you can do that even if you pass {x:7} as the this binding to the script, since the script can just create a new function, call it, and access 'this' from there to get at the global object | 11:06.54 |
| | 'this' at the top scope of the script resolves to the global object *ONLY* in non-strict mode | 11:07.09 |
| | this at the top scope in strict mode is undefined | 11:07.19 |
| avih | oh, i thought that's only for function scopes | 11:07.30 |
| | but then, a script is a function, of sorts | 11:07.44 |
| | (sec, digesting) | 11:08.13 |
| tor8 | or at least it should be, I could have a bug where I actually pass the global object (a remnant of how I handled this previously) | 11:08.26 |
| | yeah, js_dostring and js_dofile have a pushglobal instead of pushundefined for the this binding. | 11:09.05 |
| | I should ifx that. | 11:09.07 |
| avih | (still digesting :) ) | 11:09.29 |
| tor8 | 'this' is a massively ugly hack in JS | 11:09.48 |
| avih | indeed | 11:09.53 |
| | this i know, pardon the pun | 11:10.03 |
| tor8 | basically, if you invoke a function from a member expression, the first half is used as the binding for this | 11:10.15 |
| | otherwise this is undefined | 11:10.18 |
| | and accessing this, if it's undefined gets the global object (in non-strict mode) | 11:10.29 |
| | so if you invoke a function any other way, this is undefined | 11:10.46 |
| | and then there are hacky workarounds like Function.prototype.apply and .call to manually set the this binding to get around this when you want to call a function pointer with a this binding | 11:11.18 |
| | and this is why you get tutorials advocating 'var that = this;' at the top when emulating Java-style class inheritance | 11:12.04 |
| avih | i still feel somehow that if you run a new script (js_loadstring) with a custom this, then it should be its global. but i'm not entirely confident about that assertion | 11:12.09 |
| tor8 | you mean you want to run a script with a custom object as the global environment? | 11:12.35 |
| avih | "should" as in "it would be a nice behavior of the api" | 11:12.47 |
| | yes | 11:13.00 |
| tor8 | avih: look at js_loadeval() | 11:13.55 |
| avih | but, i'm not familiar enough with the intricacies of the global object to say that with confidence | 11:14.31 |
| tor8 | normally a script loaded with js_loadscript uses the global object as its scope | 11:14.37 |
| | with js_loadeval it uses the current function's scope | 11:14.47 |
| avih | agreed | 11:14.48 |
| tor8 | so any variables it sets are set in the current function's scope, not in the global object | 11:15.10 |
| avih | but it can still access the global object | 11:15.35 |
| tor8 | but js_loadeval is really only intended to be called from eval(). there's a lot of gotchas with scoping and C functions. | 11:15.41 |
| avih | hmm | 11:15.58 |
| tor8 | yes. there's no way to avoid that unless you create a new js_State context. | 11:16.05 |
| | if you want to prevent the scripts from clobbering the global object with their crappy global variables, then eval is the way to go | 11:16.32 |
| avih | but then the C code can't share objects between the states | 11:16.33 |
| tor8 | true. there's no mechanism to call a script/function with a custom global object. | 11:17.30 |
| avih | i don't mind scripts (modules) clobbering the global object. that's up to the user and the scripts he runs | 11:17.38 |
| | i do make sure their declared vars don't clobber the global though, since they run in a function scope | 11:18.07 |
| | tor8: so the only effect of a this object which is not global (or an equivalent undefined), is that in non strict mode the top-scope of the loaded string 'this' will be that object. and that's it. right? | 11:20.09 |
| tor8 | I don't understand that question. | 11:21.12 |
| | the value you pass as this when calling the script object, is the value that any 'this' expression will access (and if in non-strict mode, a value of undefined or null will be promoted to the global object by the runtime) | 11:22.32 |
| avih | i'm trying to realize the exact extent that the 'this' object has on js_loadstring. my understanding that the only difference is: 1. that string is in non strict mode. AND 2. the value of 'this' which it sees at its top lexical scope would be that custom object. | 11:23.14 |
| tor8 | 'this' is not part of the lexical scope. | 11:24.04 |
| | it's a dynamic binding | 11:24.13 |
| | it is NOT captured by lexical closures | 11:24.24 |
| avih | i.e. if the vm is in strict mode to begin with, or if the string uses 'use strict'; at its top lexical scope then there would be no difference if i js_loadstring with {x:7} or pushglobal or pushundefined | 11:24.30 |
| tor8 | which leads to contortions like "var that = this" in order to capture the initial 'this' value lexically | 11:24.43 |
| avih | is there another way to interpret "its this at the top lexical scope" ? | 11:25.17 |
| tor8 | if you call the script object loaded with js_loadstring with this set to {x:7} then 'this' will resolve to {x:7} REGARDLESS of strict mode | 11:25.24 |
| avih | it's the thing which is evaluated when it uses an unbounded "this" | 11:25.40 |
| tor8 | if you call it with pushglobal, 'this' will be the global object REGARDLESS of strict mode | 11:25.51 |
| avih | oooohhhhhhhhhh | 11:26.06 |
| tor8 | if you call it with pushundefined, 'this' will be the global object if non-strict, otherwise it will be undefined | 11:26.09 |
| avih | so, with js_loadstging _ pushundefined + call, the top scope will see the global in non strict and undefined if it sets strict. | 11:28.26 |
| tor8 | correct. | 11:28.39 |
| avih | 2. with pushglobal, "var x = this" will always assign the global object to x regardless if the state has strict mode or the script sets scrict mode | 11:29.09 |
| tor8 | yes. | 11:30.00 |
| avih | and 3. after pushing a this of {x:7}, in non strict mode this will be {x:7}, in strict mode it would be undefined. | 11:30.02 |
| tor8 | no. why do you think it would be undefined in strict mode? | 11:30.27 |
| avih | because i didn't finish digesting your comments earlier? :) | 11:31.06 |
| tor8 | jsrun.c, line 1323. case OP_THIS: | 11:31.22 |
| | that's the byte code operator for resolving a "this" expression. | 11:31.41 |
| | it pushes the value of 'this' to the stack | 11:31.58 |
| | js_iscoercible returns true if a value is null or undefined | 11:32.16 |
| | eh, false* | 11:32.29 |
| | i.e. can the value safely be promoted to an object | 11:32.37 |
| | aka coerced | 11:32.44 |
| | so: whatever you stick in the 'this' slot when calling a function is what will be returned for a 'this' expression | 11:33.19 |
| avih | so non-strict mode means (in this conversation context): "if i'm invoked with an undefined this object, i shall use the global object instead"? | 11:33.31 |
| tor8 | unless it happens to be null or undefined, and you happen to be non-strict. | 11:33.35 |
| | YES! | 11:33.52 |
| avih | (when assigning var x = this) | 11:33.53 |
| | a-ha! | 11:34.00 |
| | but anything which isn't undefined is returned when asssigning this to something | 11:34.31 |
| | used* | 11:34.50 |
| | or null | 11:35.17 |
| tor8 | yes. whatever you put in the 'this' slot is what will get used as 'this' (unless you happen to be non-strict and it happens to be undefined, in which case the VM substitutes in the global object) | 11:35.53 |
| avih | i am ALWAYS non strict, unless someone sets it otherwise. | 11:36.19 |
| | so "it's ABC, unless you happen to be non strict" to me is "t's ABC. no it's not" :) | 11:37.24 |
| | but anyway, yeah, i think i got it eventually :) thanks. | 11:39.05 |
| | tor8: btw, when you say "try fixing it in jsparse.c ..." it means i can only test it with that fix, but i don't expect users to get my copy of mujs. i expect them to install it on their own from their distro/git/whatever. so me fixing it won't help users. | 11:52.47 |
| | in fact, i don't have a copy of mujs to distribute. it links with libmujs if it can, and doesn't enable js if it can't | 11:53.29 |
| tor8 | avih: it means "try that and please tell me if it works" | 11:56.34 |
| avih | right, that i can do :) | 11:56.52 |
| tor8 | since it worked, I've already pushed a commit with that fix. | 11:57.10 |
| avih | :) | 11:57.16 |
| | i only tested with this testcase though. | 11:57.31 |
| | tor8: js_do* should print a stack trace too | 12:00.30 |
| tor8 | they do with the Error.prototype.toString implementation in main.c | 12:01.13 |
| | though I guess if I change js_dostring I can remove Error.prototype.toString | 12:01.59 |
| | but we can't be certain that the error object thrown in js_dostring is an actual Error object with a stack trace so I'd rather not | 12:02.56 |
| avih | but dostring is an api. you don't have main.c when using the lib | 12:04.13 |
| | ok. toString is ok i guess. whoever wants trace should do the legwork | 12:05.13 |
| | tor8: actually, you advertise js_dostring as safe, but then js_tostring (in its catch) could easily throw too, and this will crash the caller which assumes it never throws | 12:09.35 |
| | (that's the exact scenario for which i designed my "autowash" system) | 12:10.57 |
| tor8 | avih: yes, I'm tinkering with some ideas to get around that problem | 12:13.36 |
| avih | just js_try. if you can't get the string, use "Unknown error" | 12:14.01 |
| | and the docs on mujs.com are really outdated. | 12:15.40 |
| | (i should really read the ES5 spec from start to finish at least once) | 12:22.47 |
| Robin_Watts | sebras: 3 commits on robin/mater | 18:09.16 |
| sebras | Robin_Watts: CMAP dict free LGTM. | 18:16.22 |
| Robin_Watts | (Cluster test completes with no differences) | 18:17.50 |
| sebras | Robin_Watts: in the last patch where you fix the default value of destination alpha, why aren't you also changing the next to last argument when calling template_solid_color_N_general() in paint_solid_color_N_da(). | 18:21.10 |
| | it seems asymmetrical. | 18:21.15 |
| Robin_Watts | da is 0 or 1. sa is an alpha value (0 to 256) | 18:21.53 |
| | or am I misunderstanding? | 18:22.13 |
| sebras | I think so, I see that you are fixing the case of 0, 1, 3, 4 components, but not the N components case. | 18:22.45 |
| | oh, and you probably mixed sa and da in the sentence above, right? | 18:23.27 |
| | at least your patch changes the da parameters to a default value of 256. | 18:23.49 |
| Robin_Watts | sebras: Ignore that patch for a mo. | 18:24.28 |
| sebras | ok. | 18:24.48 |
| | Robin_Watts: the fz_image leak commit is not so clear for me. | 18:26.48 |
| | what is the problem it is fixing. | 18:26.54 |
| | even without understanding it, the return value of fz_drop_key_storable_key() is cast to void, while the return value of fz_drop_key_storable() is not. | 18:28.06 |
| Robin_Watts | ok, updated sa patch up there. | 18:28.42 |
| sebras | it seems to me like this patch fixed one bug in gprf-doc, one in how we interact with mememtno and one with the reference counting realting to stored images..? | 18:29.05 |
| Robin_Watts | sebras: OK, so that one... | 18:29.21 |
| | As I was debugging the new plotting code I hit a place where memento was leaking. | 18:29.40 |
| | When images are dropped, we call fz_drop_image. | 18:29.55 |
| | And if the reference hits zero, we call a supplied 'drop' function. | 18:30.32 |
| sebras | yes, agreed. | 18:30.41 |
| Robin_Watts | fz_drop_image | 18:30.51 |
| | That used to call fz_drop_key_storable, and if it actually dropped anything, would then free some other stuff. | 18:31.34 |
| sebras | yes, depending on if the store would allow it to be dropped. | 18:32.18 |
| Robin_Watts | The whole point of fz_drop_key_storable, is that these objects can also be dropped from the store. | 18:32.20 |
| | sebras: Urm... no. it's not whether the store 'allows' it to be dropped. | 18:32.43 |
| sebras | Robin_Watts: but whether it could be dropped depended on if the image was used as a key, or something like that. my memory is hazy on this. | 18:33.19 |
| Robin_Watts | If we drop the last reference, then it WILL be freed. | 18:33.30 |
| | Internal to a key_storable, we have another 'drop' function, that is called if we actually need to do the drop (i.e. if the reference hits zero) | 18:34.18 |
| | hence it's silly to do some of the freeing in that function, and some of it externally. | 18:34.40 |
| | (in fact, it's worse than silly, it's unsafe - if the store decides to drop the key_storable then it doesn't do the external bit of the freeing) | 18:35.10 |
| | Hence I've moved everything into the innermost drop function. | 18:35.21 |
| | The innermost drop functions are fz_drop_image_imp and fz_drop_image_gprf_imp | 18:35.56 |
| | and they both rely on the new fz_drop_image_base function. | 18:36.08 |
| | So it would probably be best if I stopped fz_drop_key_storable returning an int. | 18:37.10 |
| | Also, while I was debugging this stuff, I noticed that the Memento 'history' of block actions was missing an addRef, hence the addition of the extra memento call. | 18:39.09 |
| | Let me write a better commit message. | 18:39.50 |
| sebras | aha, so the change in fz_drop_image_gprf_imp() is mirroring the one in fz_drop_image_imp() because they are each their own image->drop_image() callback. | 18:46.41 |
| | now I understand what changed and why this is a better solution. | 18:47.03 |
| | but yeah, a clearer commit message might have saved you the discussion. :) | 18:47.35 |
| | Robin_Watts: I know I'm picky, but I don't want to LGTM unless I understand what is happening. just looking at the syntax is pointless. | 18:48.08 |
| Robin_Watts | sebras: Not at all, you've prodded me into improving 2 of my 3 commits. | 18:48.39 |
| | OK, new commits up. | 18:49.06 |
| | and they still cluster test OK. | 18:55.50 |
| sebras | Robin_Watts: alright, I have a related question to the sa commit: in paint_solid_color_N_da() we supply color[1] as an argument to FZ_EXPAND(), later to be used as sa. that seems reasonable, but in all other places the index into color seems to correspond to int n, but not in this case. | 18:57.46 |
| | I don't understand all the details here, but your change looks reasonable. | 18:58.04 |
| Robin_Watts | color[] is an array of color values, all 0..255 | 18:58.21 |
| sebras | the related question might be another bug, or I'm just not understanding it. | 18:58.26 |
| Robin_Watts | FZ_EXPAND(x) takes x from being 0 <= x <= 255 to being 0 <= FZ_EXPAND(x) <= 256 | 18:58.54 |
| sebras | yes, the range is not what I'm asking about, I'm asking about which component of color is being used in paint_solid_color_N_da(). | 18:59.17 |
| Robin_Watts | ok, so da and sa are destination alpha and source alpha. | 18:59.54 |
| sebras | i.e. should FZ_EXPAND(color[1]) be FZ_EXPAND(color[n])? | 19:00.00 |
| Robin_Watts | da is usually 0 or 1, to indicate the presence or absence of a destination alpha plane. | 19:00.30 |
| | sebras: Where are you looking? | 19:00.58 |
| sebras | source/fitz/draw-paint.c line 464 in function paint_solid_color_N_da(). | 19:01.25 |
| Robin_Watts | I see it. | 19:01.55 |
| | Hmm, yes. | 19:02.02 |
| | Yes, I believe you're right. | 19:02.29 |
| sebras | to me it seems as if we just take the second component of N components, and it doesn't look symmetrical. | 19:02.32 |
| Robin_Watts | sebras: I'll add a commit for that too. | 19:02.58 |
| | 1 more commit for that then. Thanks. | 19:05.18 |
| sebras | Robin_Watts: ok, robin/master LGTM. | 19:08.02 |
| Robin_Watts | Thanks. | 19:08.27 |
| sebras | Robin_Watts: it is a bit interesting that template_solid_color_N_general() takes int sa as an argument | 19:09.38 |
| | when template_solid_color_4_da() does not. | 19:09.45 |
| | that's not symmetrical, but it doesn't matter for the execution. | 19:09.56 |
| Robin_Watts | The clue is in the name. | 19:10.05 |
| | general copes with the general case (sa or not, da or not) | 19:10.22 |
| sebras | right, I'm just thinking that this might be another reason we missed it. | 19:10.48 |
| | oh well, off to bed! :) | 19:10.52 |
| sebras | sleeps | 19:11.16 |
| Robin_Watts | template_solid_color_4_da() only copes with da or not. | 19:11.18 |
| | Thanks again. have a good one. | 19:11.31 |
| | Forward 1 day (to 2017/05/10)>>> | |
Log of #mupdf at irc.freenode.net.