Last Comment Bug 757908 - Remove JSRESOLVE_DECLARING
: Remove JSRESOLVE_DECLARING
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 10:48 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-06-30 12:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (5.15 KB, patch)
2012-05-23 10:48 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Slightly unbitrotted (5.01 KB, patch)
2012-06-25 12:53 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-23 10:48:28 PDT
Created attachment 626500 [details] [diff] [review]
Patch

As part of the quest to remove the non-standard concept of resolve flags, JSRESOLVE_DECLARING should be removed.

It's possible to remove this flag from Gecko and SpiderMonkey without breaking any tests; I pushed a build with the attached patch to try and got clean results (well, as clean as they ever are).  But some other embedders use it, so we need to figure out some sort of path forward for them before we can do this.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-05-23 10:50:51 PDT
Comment on attachment 626500 [details] [diff] [review]
Patch

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

::: dom/workers/RuntimeService.cpp
@@ +366,5 @@
>  {
>    AssertIsOnMainThread();
>  
> +  // Don't care about assignments, bail now.
> +  if (aFlags & (JSRESOLVE_ASSIGNING)) {

Less parens
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-25 03:12:50 PDT
Wes, what SpiderMonkey source code is gpsee compiled against?  The more I think about this, the more I think it's increasingly likely that you can just remove your |if (flags & JSRESOLVE_DECLARING) return JS_TRUE;| and nothing will change, due to bug 632003 and bug 577325 (which landed awhile ago now).  (Those two bugs are also, I suspect, the reason we can remove JSRESOLVE_WITH in bug 758499.  Man, the side effects of fixing those two bugs were so good...)

The potentially easiest way to test this hypothesis -- assuming you're on a new enough version of SpiderMonkey that those bugs were fixed in it -- seems to me to be to backport this patch to that version of SpiderMonkey (being just removals, it should be basically trivial), then have you test it with the if-declaring statement commented out.  Could you point me at the exact SpiderMonkey source you're using, then I can point you at a patch to it to test my hypothesis?
Comment 3 Wesley W. Garland 2012-05-29 12:15:02 PDT
Unfortunately, I'm on an old enough version of spidermonkey that I can't test this easily  (we have not had sufficient time to keep with API churn, but hope to dedicate some resources to this again in Q2 or Q3).

That said -- I think removing JSRESOLVE_DECLARING *will* break us, as we have what is now a fairly uncommon usage pattern.

We want to give our users scopes like this (braces are scope markers, like in C):

moduleMain{
  var a;

  moduleA1{
    var a;
  }

  moduleA2{
    var a;
  }
}

if we leak symbols from moduleA1, moduleA2 to moduleMain, we should find that moduleA1.a !== moduleA2.a,
moduleA1.a !== moduleMain.a
moduleA2.a !== moduleMain.a
moduleA1.Object === moduleA2.Object === moduleMain.Object

moduleMain, moduleA1, and moduleA2 are separate compilation units.  In JS 1.7 and JS 1.8.0, we simply constructed new "module global" objects, set up the prototype chain to allow this, and passed the compile scripts (modules) into JS_ExecuteScript() with these "module globals" as the obj ("scoping object") parameter.

With the advent of TraceMonkey, would found out that doing this caused TraceMonkey to not trace any scripts whose scoping object was not the top-level global that had the standard classes defined on it.  I haven't checked recently, but am pretty sure that current JITs still assume they can find the standard classes directly on the scoping object without travelling up the prototype chain.

Our solution -- which I knew at the time was not necessarily long-term viable -- was to essentially refactor the above program into
moduleMain{
  var a;
}

moduleA1{
  var a;
}

moduleA2{
  var a;
}

... each module gets it own bona-fide Global object, with a non-default resolver, and then when moduleA1 or moduleA2 tries to resolve a value on this global, we reach into the global object belonging to moduleMain, and JS_DefineProperty() it on moduleA1/A2's scoping object with same jsval.  (A little more magic with JSOP_INIT too).

This works, but absolutely depends on JSRESOLVE_DECLARING -- otherwise we would not know the difference between "var a=0" and "a=0", which is a pretty big difference in our environment.

I am not currently aware of any way to implement our current semantics without refactoring the JavaScript which makes up the module source code when it is loaded, to add a dummy closure around it.  This dummy closure could then take named arguments which we inject with the appropriate values when the module loads. Ugly but probably functional; not sure if perf will be affected or not.

I guess maybe this points that there is a weakness in the JSAPI? - It would be nice, from an embedder's POV, to be able to define variables in a scope, other than "global", but there is no way to get a handle on a lexical scope.  It would be nice if we could somehow manipulate blocks of JS before they are executed.  Kind of like web devs manipulate DOM trees before they are attached to the document.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-25 12:53:47 PDT
Created attachment 636450 [details] [diff] [review]
Slightly unbitrotted

Hmm.  So Wes thinks this will break him, I'm not so sure about that, and there's no easy way to test the hypothesis.  And he's a couple revs behind now and won't be updating to deal with this for a couple quarters or so.  I guess let's go ahead with this now, then, and help him deal with the fallout -- if there is any at all -- when we get to it.  :-\
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-29 22:22:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc305a9ac000
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-06-30 09:58:54 PDT
Sorry, this got caught up in an Android XUL browser-chrome orange investigation. It was backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc01c494f6a.

It was re-landed in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db570d1247ee

Apologies for the churn.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-06-30 12:40:22 PDT
https://hg.mozilla.org/mozilla-central/rev/db570d1247ee

Note You need to log in before you can comment on or make changes to this bug.