Jeff Walden [:Waldo] Assignee
	Description • 12 years ago

Resolve flags need to go away as they don't exist in specs, and they impose complexity on every property access.  JSRESOLVE_QUALIFIED is just the latest of them to hit the chopping block.  (Only two to go now!)

There are only a very few places that use JSRESOLVE_QUALIFIED.  Unfortunately each tends to involve its own complexity, and its own sad backstory, and its own special snowflake solution or hackaround.  So there's going to be a series of small patches here.

A sneak preview of the patch series is currently running through try.  Seeing as these have been kind of intricate to work through, and I'm not at all confident I *have* worked through all of them, I make no guarantees these particular patches will eventually be posted here:

https://tbpl.mozilla.org/?tree=Try&rev=a61ecb1e59b0

	Luke Wagner [:luke]
	Comment 1 • 12 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #0)
> (Only two to go now!)

\o/

	Jeff Walden [:Waldo] Assignee
	Comment 2 • 12 years ago

I declare Green Enough; patches commence.

	Jeff Walden [:Waldo] Assignee
	Comment 3 • 12 years ago

There's no spec analog, and nothing in our test suite breaks when I do this.  As far as the web goes, who knows.  I could let it sit a couple days before going further, if needed.  Given this comes closer to syncing up with what other browsers do, I'm sure, I wouldn't be too worried, myself.

Most of these patches are logically separate, so I think I could apply them in any order.  This just happened to come first in my queue, because I finished it up first.

	Jeff Walden [:Waldo] Assignee
	Comment 4 • 12 years ago

So, um, this code was really here solely to deal with |with (document) all;|?  I'm not sure what else it would have been.  (hasOwnProperty and in should have been qualified.)  Anyway, tests were green with this patch, so this should be good.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 12 years ago

Comment on attachment 694152 [details] [diff] [review]
Remove the concept of writable-[Replaceable]

r=me with the following caveats:

1)  Some of those jsids are probably unused now and the corresponding static can go away.  Please kill any that are thus unused.

2)  Add a test that tests that the new behavior is not replaceable (e.g. assigning "abc" to unqualified innerHeight should not end up setting the property to "abc", since the setter takes int).  Similarly for assigning {} to status.

3)  Please double-check what other UAs are in fact doing on that test you write.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

Comment on attachment 694154 [details] [diff] [review]
Don't check for JSRESOLVE_QUALIFIED when resolving document.all

> So, um, this code was really here solely to deal with |with (document) all;|?

No, it was also there to deal with:

  <script>
    var all = 5;
  </script>
  <div onclick="alert(all);">Click me</div>

It looks like in that scenario other browsers will find the collection object, not the var (while we currently alert 5), so I guess this change is ok.  But we should add a test for this too, ideally.  r=me

	Jeff Walden [:Waldo] Assignee
	Comment 7 • 12 years ago

Okay, this one's kind of complicated.  foo = ...| ordinarily, if there's no |foo| property already, will add a new |foo| property to the global object.  That new property JSClass.getProperty and JSClass.setProperty from the global's class as its getter/setter PropertyOp functions.  But since those are overridden for |window| and are not PropertyStub/StrictPropertyStub, those properties become Slow.

To avoid this slowdown we have a long-standing hack to instead add Stub properties.  What happens is the global's resolve hook, when asked if the property exists, checks if it's being called for a |foo = ...| resolution using JSRESOLVE_QUALIFIED.  (It also checks for an assignment happening, using JSRESOLVE_ASSIGNING, which is equally bad.  But one step at a time.)  If it's a |foo = ...| case, we resolve a data property with value |undefined| into place.  Then the normal property-setting code kicks into gear and overwrites that value with the assigned one.

There are two issues with this.  First, since we're making the property exist when the JS engine's testing if it did exist, we're going to short-circuit any code that would warn (through JSOPTION_STRICT) about assigning to an undeclared variable.  Meh, whatever, ish, it's non-standard warning behavior.  But second, in short-circuiting that, we also short-circuit code that enforces ECMAScript strict mode semantics, where assignment to an undeclared global throws a TypeError.  Which matters a bit more.

So as a hackaround to the hackaround, the DOM |window| hackaround code calls a friend API function, for an unqualified access, that checks for JSOPTION_STRICT and strict mode and will warn or throw a TypeError as appropriate.  Hackaround.

There's an easy solution to removing use of JSRESOLVE_QUALIFIED here: just make the friend API function check for qualification itself.  It already has to get the relevant script to check for strict mode code, and the PC is readily available.  So it can just sniff for unqualified bytecode ops, and the JSRESOLVE_QUALIFIED can go away.  This patch implements that.

This is a one step forward, nine-tenths of a step backward patch.  Getting |window| off implementing this fast-path hack, by making the global use Stubs for its get/set, is the best fix here.  But in the meantime this is progress.  And given we already have a friend API hackaround in use here, it's really no skin off our noses to add a little extra fugly to it to make it more discerning.

The newly-renamed MaybeReportUndeclaredVarAssignment and js::ReportIfUndeclaredVarAssignment have some similar code to them.  But given both are undesirable in the longer run, and the latter will hopefully disappear pretty soon when window doesn't use JSClass.{g,s}etProperty at all, it seems kind of pointless to common anything up.

I'm setting two reviewers to be safe, given that I'm not especially certain of the correctness of the stack inspection code or that I've necessarily tested for all the correct ops.  :-)

	Jeff Walden [:Waldo] Assignee
	Comment 8 • 12 years ago

I didn't tryserver this bit because I wasn't confident enough in the rest of the patches, but with those good, this too should be good by simple enough inspection.

	Luke Wagner [:luke]
	Comment 9 • 12 years ago

Comment on attachment 694160 [details] [diff] [review]
Don't use JSRESOLVE_QUALIFIED to handle warning on undeclared-var accesses

Review of attachment 694160 [details] [diff] [review]:
-----------------------------------------------------------------

Heh, I've seen pieces of this hackaround story before; nice to hear the whole tragedy.

::: js/src/jsobj.cpp
@@ +4452,5 @@
> +js::ReportIfUndeclaredVarAssignment(JSContext *cx, HandleString propname)
> +{
> +    {
> +        jsbytecode *pc;
> +        UnrootedScript script = cx->stack.currentScript(&pc);

I think you'll want currentScript(&pc, ContextStack::ALLOW_CROSS_COMPARTMENT) for the same reason as MaybeReportUndeclaredVarAssignment.  Now, maybe MaybeReportUndeclaredVarAssignment doesn't actually need ALLOW_CROSS_COMPARTMENT (I'm having a hard time conceiving of how a cross-compartment scope name access can occur), so, either way, I'd keep them the same.

@@ +4472,5 @@
> +            *pc != JSOP_SETNAME && *pc != JSOP_SETGNAME &&
> +            *pc != JSOP_INCNAME && *pc != JSOP_INCGNAME &&
> +            *pc != JSOP_NAMEINC && *pc != JSOP_GNAMEINC &&
> +            *pc != JSOP_DECNAME && *pc != JSOP_DECGNAME &&
> +            *pc != JSOP_NAMEDEC && *pc != JSOP_GNAMEDEC)

What about JSOP_CALLNAME and JSOP_CALLGNAME?  If "yes" to these, then could you just use "JOF_MODE(format) == JOF_NAME" instead?  Technically, local, arg and aliasedvar access is also JOF_NAME but these will never ever result in a dynamic name lookup.

	Luke Wagner [:luke]
	Comment 10 • 12 years ago

Comment on attachment 694161 [details] [diff] [review]
Remove all remaining JSRESOLVE_QUALIFIED uses, and the very few places that still test for it in a diagnostic manner

Good luck!

	Jeff Walden [:Waldo] Assignee
	Comment 11 • 12 years ago

(In reply to Luke Wagner [:luke] from comment #10)
> Good luck!

http://www.youtube.com/watch?v=gQJGpmOzfpA (and sigh over bad video)
http://www.youtube.com/watch?v=SmHeP9Sve48

	Luke Wagner [:luke]
	Comment 12 • 12 years ago

Yep, that's about what I meant (but I won't tell you which ;)

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 12 years ago

Comment on attachment 694160 [details] [diff] [review]
Don't use JSRESOLVE_QUALIFIED to handle warning on undeclared-var accesses

r=me.  I'm really sorry about this ugly.  We will nuke it, I promise!

	Jeff Walden [:Waldo] Assignee
	Comment 14 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0350335cab3 for the unqualified-all hunk, although I had to futz with a test a bit before getting one that worked.  Turns out, some browsers put an "all" property on every element, so in IE/Opera, that testcase would alert a value *not* document.all.  I made the message associated with the is() note this possibility, so eventually when browsers align on something here, the possibility will be obvious in the test.

Various other bits of work are still needed on the other hunks before they can land, but it should happen Soon.

	Luke Wagner [:luke]
	Comment 15 • 12 years ago

(In case you missed it: I'm waiting on an answer to the question in comment 9.)

	Jeff Walden [:Waldo] Assignee
	Comment 16 • 12 years ago

(In reply to Luke Wagner [:luke] from comment #9)
> > +        UnrootedScript script = cx->stack.currentScript(&pc);
> 
> I think you'll want currentScript(&pc,
> ContextStack::ALLOW_CROSS_COMPARTMENT) for the same reason as
> MaybeReportUndeclaredVarAssignment.  Now, maybe
> MaybeReportUndeclaredVarAssignment doesn't actually need
> ALLOW_CROSS_COMPARTMENT (I'm having a hard time conceiving of how a
> cross-compartment scope name access can occur), so, either way, I'd keep
> them the same.

Hmm, can't really hurt.  And we'll get rid of this garbage soon enough, anyway, when all the caching stuff is disentangled from the actual property modification mechanism, so it doesn't really matter if it's fuglied up more now.

> What about JSOP_CALLNAME and JSOP_CALLGNAME?  If "yes" to these, then could
> you just use "JOF_MODE(format) == JOF_NAME" instead?

The classinfo caller is currently guarded by a JSRESOLVE_ASSIGNING, so those couldn't be hit.  Come to think of it, JSOP_NAME and JSOP_GETGNAME can't be hit if that's the case, can they.  Removed those as well.  And, um, the dec/inc ops are only for the benefit of That Which Shall Not Be Named, so I can remove those too, can't I.

This is currently running through a limited try-run to be safe:

https://tbpl.mozilla.org/?tree=Try&rev=fb253a131f58

	Jeff Walden [:Waldo] Assignee
	Comment 17 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d09962f1e79

	Jeff Walden [:Waldo] Assignee
	Comment 18 • 12 years ago

And the writable-[Replaceable] patch, after a little discussion in newsgroups of the change, without dissent from what this patch does, and with a test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b8f166c3b5

Given this particular bit is potentially twitchy, and there's not really a huge rush, I'll probably push The Final Removal(tm) next week, after a few days of baking and all.  (I'd like to get at least a little waiting in before that, because that patch touches/removes every other use of JSRESOLVE_QUALIFIED, which would make this bit trickier to adjust if necessary.)

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 19 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/d0350335cab3
https://hg.mozilla.org/mozilla-central/rev/8d09962f1e79
https://hg.mozilla.org/mozilla-central/rev/c3b8f166c3b5

	Jeff Walden [:Waldo] Assignee
	Comment 20 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7da4138801e
https://hg.mozilla.org/integration/mozilla-inbound/rev/268fe9874d9d

A typo fix, then the final patch, and JSRESOLVE_QUALIFIED is now gone.  \o/  Too early to start dancing on its grave?  Probably, alas.  :-\  But I remain cautiously optimistic!

	Paul Wright
	Comment 21 • 12 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c7da4138801e
> https://hg.mozilla.org/integration/mozilla-inbound/rev/268fe9874d9d
> 
> A typo fix, then the final patch, and JSRESOLVE_QUALIFIED is now gone.  \o/ 
> Too early to start dancing on its grave?  Probably, alas.  :-\  But I remain
> cautiously optimistic!

Does this mean the "leave open" in the whiteboard can be removed?

	Jeff Walden [:Waldo] Assignee
	Comment 22 • 12 years ago

I noted JSRESOLVE_QUALIFIED as obsolete on the main JSAPI reference page, and on the JSNewResolveOp page that actually describes its semantics.  Which means all that's necessary now is to document its removal in a JSAPI 20 release notes page, or something like that.  Marking d-d-n for that eventuality.

...and yeah, this is no longer leave-open territory, thanks for pointing that out.  :-)

	Paul Wright
	Comment 23 • 12 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)

> 
> ...and yeah, this is no longer leave-open territory, thanks for pointing
> that out.  :-)

Hey, you did the heavy lifting... I just read stuff  :-)

	Graeme McCutcheon [:graememcc]
	Comment 24 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/c7da4138801e
https://hg.mozilla.org/mozilla-central/rev/268fe9874d9d