673569 - Let each frame script have its own anonymous scope

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

I'm going through the current existing framescripts, and as they all run in the same JS context, they end up poluting the scope with their functions, and fighting for who defines Cc,Ci first.

This wraps the script in an anonymous function to make sure only the object we want to expose is added.

I ran Harness_sanity and it passes

Comment 1

[(not currently active) Ted Mielczarek]

Could this be the root cause of bug 668471?

Comment 2

[(not currently active) Ted Mielczarek]

Also, this is unfortunate, I certainly didn't know that all frame scripts would share the same global! Is that something we can change, or do existing scripts rely on that behavior? If we're going to migrate desktop Firefox to e10s, lots of things are going to be using frame scripts, seems like a recipe for more pain (much like existing extensions overlaying the browser window).

Comment 3

[(not currently active) Ted Mielczarek]

These were added to specialpowers.js in bug 641706:
http://hg.mozilla.org/mozilla-central/rev/132e89233cfa

What other frame script are they conflicting with now?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

It would be quite overkill to create separate global for each frame scripts.
But, we could add a new parameter to loadFrameScript.

-void loadFrameScript(in AString aURL, in boolean aAllowDelayedLoad);
+void loadFrameScript(in AString aURL, in boolean aAllowDelayedLoad,
                      [optional] in boolean aAnonymousScope);

If the last parameter was true, then the script could be
run inside (function() { [script] })()
Or perhaps JS eng has some helper method for that.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: anonscope

Felipe, could you try this patch and make special powers to load frame script
using anonscope?

Comment 6

[cmtalbert]

Comment on attachment 547810 [details] [diff] [review]
Patch

This is ok, but I'd prefer that we use :smaug's anonscope method in his patch on this bug as it seems like a cleaner approach.  Can we try that and see if it gets us the same benefit.  If so, then this patch may be unnecessary.  Holding off on review until we know if smaug's approach will work.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 548241 [details] [diff] [review]
anonscope

Works great!

SpecialPowers already attaches itself to the window (through observers), so I don't even have to expose it through `this`. I just have to set the new parameter of loadFrameScript to true to make the "Cc redeclaration" warnings go away.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #4)
> It would be quite overkill to create separate global for each frame scripts.
> But, we could add a new parameter to loadFrameScript.
> 
> -void loadFrameScript(in AString aURL, in boolean aAllowDelayedLoad);
> +void loadFrameScript(in AString aURL, in boolean aAllowDelayedLoad,
>                       [optional] in boolean aAnonymousScope);

I do not prefer multiple boolean parameters in one method like this since it is difficult to read what the second boolean value means at the caller side.

Can we add a new method loadFrameScriptinAnonymousScope or define enums like ALLOW_DELAYED_LOAD and LOAD_IN_ANONYMOUS_SCOPE?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I don't like such enums, but another method might be good.

Though, DOM APIs are full of boolean parameters.
add/removeEventListener, importNode, etc.
and IMO we should model message manager APIs to work "similarly" to
DOM APIs.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 548241 [details] [diff] [review]
anonscope

Benjamin, what do you think?

The patch itself is pretty mechanical change, though
there is the script caching related thing that
if someone does first loadFrameScript(someurl, true, true);
and then loadFrameScript(someurl, true, false);
the latter doesn't use script caching, since the cache
uses currently url as key, and the value differ depending on
whether the script should run in global or anon scope.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Apparently the patch may have some problem, but I and felipe
will figure that out.

Comment 12

[Benjamin Smedberg]

Comment on attachment 548241 [details] [diff] [review]
anonscope

What would happen if we just gave all frame scripts their own scope? Or at least made that the default? Would that break the way Fennec uses them currently?

(I'm not worried about the cacheing thing at all: I cannot imagine a correct usecase where you'd run the same script with different scoping parameters.)

Comment 13

[Josh Matthews [:jdm]]

It's worth noting that in the current situation, errors like the following in addonManager.js:

>messageManager.loadFrameScript(CHILD_SCRIPT, true, true);

will cause the scripts to be loaded into anonymous scopes.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Olli, apparently I cannot reproduce the formSubmitListener error, but I can reproduce the InstallTrigger one.

With the patch, and runInAnonScope set to true (as it already is, as pointed in comment 13), the script runs but this listener fails to be called:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions-content.js#247

I'm using this test to try:
$ TEST_PATH=browser/base/content/test/browser_bug553455.js pymake mochitest-browser-chrome

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I have no idea in which directory to run that command

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Ok, I have better patch coming which gives the same behavior with tests
as the original patch for this bug.
But, there is still something going wrong somewhere.
Even just with the original patch browser_bug553455.js hangs occasionally.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 549178 [details] [diff] [review]
patch

Oops, wrong bug.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #12)
> What would happen if we just gave all frame scripts their own scope? Or at
> least made that the default? Would that break the way Fennec uses them
> currently?

If we made that the default we'd need to change some Fennec scripts, but that's probably straightforward (or, even easier, we can use the boolean pref to not switch this for the existing fennec scripts).

*but*, it would also mean breaking this API compat. with firefox 4-7, which would make it harder to write add-ons now that are forward-compatible with e10s.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #17)
> Created attachment 549117 [details] [diff] [review] [diff] [details] [review]
> patch

Sent this patch to tryserver and turned anon-scope for formSubmitListener and extensions-content, all tests passed.

(there are two open bugs about existing orange on browser_bug553455.js, so the timeouts are likely unrelated)

Comment 22

[Benjamin Smedberg]

I really think we'd be better off breaking the API compat here, especially since the design pattern should be that each content script is independent and manages its own state and messages. Any shared state should probably be managed with JSMs. Soon or now we should remove support for a shared scope entirely. Do we know what Fennec needs shared scope for currently?

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

I think breaking API compat would be OK for this issue. Most developers don't even realize that frame scripts can share state. Making this explicit would "fit" the mindset better.

We would need to fix some Fennec scripts, but I'm sure Felipe already has a patch somewhere :)

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

So, what should be done here?
I'm not familiar with Fennec code, so would be rather hard for me to fix that code.

I could update the patch so that anon-scope would be the default.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

So if everyone is fine with the breaking API change, I'll write the changes needed in Fennec code :)

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Also, if we are changing the default to anon-scope, maybe we should just drop the option altogether and always use anon-scope?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Attachment: default to anonScope

Comment 28

[Benjamin Smedberg]

Comment on attachment 553937 [details] [diff] [review]
default to anonScope

Is this patch sufficient or are there also mobile/ changes required? If there aren't changes required, can we just always use the anonymous scope and remove the flag entirely? That would avoid a lot of the mechanical changes here. r=me either way.

I think a JS peer should also review nsFrameScriptExecutor::LoadFrameScriptInternal if they have not already.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

felipe said that he'll fix Fennec code.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 553937 [details] [diff] [review]
default to anonScope

What does the JS API usage look to you mrbkap?
It should be pretty similar to XBL.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 553937 [details] [diff] [review]
default to anonScope

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

This looks fine to me. My only comments deal with error handling.

::: content/base/src/nsFrameMessageManager.cpp
@@ +752,5 @@
> +        }
> +        if (!holder->mRunInGlobalScope) {
> +          JSObject* method = JS_CloneFunctionObject(mCx,
> +                                                    holder->mObject,
> +                                                    global);

This can fail due to OOM.

@@ +756,5 @@
> +                                                    global);
> +          jsval rval;
> +          (void) JS_CallFunctionValue(mCx, global,
> +                                      OBJECT_TO_JSVAL(method),
> +                                      0, nsnull, &rval);

This means that we're guaranteed to be coming from the event loop here, right? So there can't be other JS code that might want to catch an exception here?

@@ +827,5 @@
> +        if (!aRunInGlobalScope) {
> +          jsFun = JS_CompileUCFunctionForPrincipalsVersion(mCx, nsnull, jsprin, nsnull,
> +                                                           0, nsnull,
> +                                                           (jschar*)dataString.get(),
> +                                                           dataString.Length(),

This can also fail (e.g. for a compile error).

@@ +858,5 @@
>            }
> +          if (!aRunInGlobalScope) {
> +            JSObject* method = JS_CloneFunctionObject(mCx,
> +                                                      scriptObj,
> +                                                      global);

Ditto.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +comments

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 555513 [details] [diff] [review]
+comments

It might be worth factoring out the error reporting into a common function. r=me either way.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Sent updated patch to tryserver together with bug 682048
https://tbpl.mozilla.org/?tree=Try&rev=716dcf63201d

Comment 37

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think we should revive this bug. We're going to be asking addon developers to use the message manager pretty soon, and this is a big problem with our API. I'm guessing that now, instead of worrying about fennec breaking, we have to worry about b2g. Does anyone know if there are similar issues there, or who to ask?

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Most probably there are issues in b2g. It uses message manager a lot more than Fennec ever did.
And desktop FF uses mm too.

Comment 39

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rebased for 2013

I rebased this and pushed it to tryserver.
https://tbpl.mozilla.org/?tree=Try&rev=9db5b2a92538
It seemed to run okay for normal browsing, but I suspect there will be problems.

Comment 40

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is my latest try push. I can't seem to get b2g working. I tried loading every script I could find with the "load into global" option, but it still fails. The logs are hard to understand and I can't figure out how to run b2g marionette tests.

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

Comment 41

[[:fabrice] Fabrice Desré]

Bill, I think you also need to change it there:
https://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.jsm#88

When running OOP we load forms.js with loadSubscript at https://mxr.mozilla.org/mozilla-central/source/dom/ipc/preload.js#92 - is that similar wrt to scoping issues?

Also, I'd like to make sure that we don't regress memory usage too much with these patches.

Comment 42

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, pushed again with the keyboard change.
https://tbpl.mozilla.org/?tree=Try&rev=016e154839ea

I don't think this patch should affect memory usage in any measurable way. It doesn't actually use a separate global. It just wraps content scripts in (function() {...})().

Comment 43

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: content-script-scoping

Thanks Fabrice. That worked. Here is the main patch. Olli, can you do the main review? I know you wrote the original patch, but that already got reviewed, so this doesn't seem unreasonable to me. Jeff, can you review the JSAPI stuff?

Comment 44

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I posted the JS changes to bug 682048.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 831925 [details] [diff] [review]
content-script-scoping

>-  nsRefPtr<nsDOMStringList> scripts = new nsDOMStringList();
>+  JS::Rooted<JSObject*> array(aCx, JS_NewArrayObject(aCx, mPendingScripts.Length(), nullptr));
>+  NS_ENSURE_TRUE(array, NS_ERROR_OUT_OF_MEMORY);
> 
>   for (uint32_t i = 0; i < mPendingScripts.Length(); ++i) {
>-    scripts->Add(mPendingScripts[i]);
>+    JS::Rooted<JSString *> url(aCx);
JSString*

> nsFrameScriptExecutor::TryCacheLoadAndCompileScript(const nsAString& aURL,
>-                                                    CacheFailedBehavior aBehavior)
>+                                                    bool aRunInGlobalScope,
>+                                                    bool aShouldCache,
>+                                                    JS::MutableHandle<JSScript*> scriptp,
>+                                                    JS::MutableHandle<JSObject*> funp)
aFoo naming for params please

>+        if (!fun)
>+          return;
{} with if, always
Same also elsewhere


>-struct nsFrameJSScriptExecutorHolder
>+struct nsFrameScriptObjectExecutorHolder
> {
>-  nsFrameJSScriptExecutorHolder(JSScript* aScript) : mScript(aScript)
>-  { MOZ_COUNT_CTOR(nsFrameJSScriptExecutorHolder); }
>-  ~nsFrameJSScriptExecutorHolder()
>-  { MOZ_COUNT_DTOR(nsFrameJSScriptExecutorHolder); }
>+  nsFrameScriptObjectExecutorHolder(JSScript* aScript) : mScript(aScript), mFunction(nullptr)
>+  { MOZ_COUNT_CTOR(nsFrameScriptObjectExecutorHolder); }
>+  nsFrameScriptObjectExecutorHolder(JSObject* aFunction) : mScript(nullptr), mFunction(aFunction)
>+  { MOZ_COUNT_CTOR(nsFrameScriptObjectExecutorHolder); }
>+  ~nsFrameScriptObjectExecutorHolder()
>+  { MOZ_COUNT_DTOR(nsFrameScriptObjectExecutorHolder); }
>+
>+  bool WillRunInGlobalScope() { return mScript; }
>+
>   JSScript* mScript;
>+  JSObject* mFunction;
Why this isn't JS::Heap<JSObject*>
>   void TryCacheLoadAndCompileScript(const nsAString& aURL,
>-                                    CacheFailedBehavior aBehavior = DONT_EXECUTE);
>+                                    bool aRunInGlobalScope,
>+                                    bool aShouldCache,
>+                                    JS::MutableHandle<JSScript*> scriptp,
>+                                    JS::MutableHandle<JSObject*> funp);
aFoo

Comment 46

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> >   JSScript* mScript;
> >+  JSObject* mFunction;
> Why this isn't JS::Heap<JSObject*>

When you use functions like JS_AddNamedObjectRoot, you don't need to use Heap<T>. Heap<T> is only for stuff that needs write barriers for the generational GC. However, the GC already knows about all roots, so write barriers aren't needed.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Right, but could you add a comment.

Comment 48

[Jeff Walden [:Waldo]]

Comment on attachment 831925 [details] [diff] [review]
content-script-scoping

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

Moderately rubberstampy.  :-)

::: content/base/src/nsFrameMessageManager.cpp
@@ +454,3 @@
>  
>    for (uint32_t i = 0; i < mPendingScripts.Length(); ++i) {
> +    JS::Rooted<JSString *> url(aCx);

Move the Rooteds out of the loop to top level, to reduce push/pop thrashing eventually.

::: content/base/src/nsInProcessTabChildGlobal.cpp
@@ +329,5 @@
>  {
>  public:
> +    nsAsyncScriptLoad(nsInProcessTabChildGlobal* aTabChild, const nsAString& aURL,
> +                      bool aRunInGlobalScope)
> +    : mTabChild(aTabChild), mURL(aURL), mRunInGlobalScope(aRunInGlobalScope) {}

Inconsistent indentation.

Comment 49

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58bfa3b59c2c

And this, because I forgot to address Jeff's comments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c3bfc7d873

Comment 50

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/90e00e373ac9 because either this or bug 673569 caused b2g emulator mochitest-5 to time out in test_innerWidthHeight_script.html, https://tbpl.mozilla.org/php/getParsedLog.php?id=31017792&tree=Mozilla-Inbound

Comment 51

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d07001e9afc

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent B2G reftest timeouts. Please make sure this has full green Try run before pushing it again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/771052f15a6c

https://tbpl.mozilla.org/php/getParsedLog.php?id=31413972&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31413760&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31413784&tree=Mozilla-Inbound

Comment 53

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e42976d8d656

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e42976d8d656

Comment 55

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The changes in this bug could definitely affect add-ons.

Comment 56

[Alexandre Poirot [:ochameau]]

It looks like it introduced some regression on b2g, if you are importing a JSM from a frame script.

child.js: (a frame script)
  Cu.import("resource://gre/modules/devtools/dbg-server.jsm"); (should export DebuggerServer)
  DebuggerServer is null
  let {DebuggerServer} = Cu.import("resource://gre/modules/devtools/dbg-server.jsm");
  DebuggerServer is defined

But the thing is that, dbg-server.jsm is a bit special, we had to workaround some weirdness of JSMs on B2G (I suspect introduced by reuseGlobal). Symbols weren't exported if you just define them in jsm global scope. You had to bind them to `this` to workaround that.
So dbg-server.jsm looks like this:
  this.EXPORTED_SYMBOLS = ["DebuggerServer"];
  this.DebuggerServer = ...;
(Note that this way of defining jsm works fine on "special-b2g" jsm behavior, but also on desktop one.)

If I convert it back to the regular way of defining jsm, it works fine:
  let EXPORTED_SYMBOLS = ["DebuggerServer"];
  let DebuggerServer = ...;

Comment 57

[Alexandre Poirot [:ochameau]]

Also note that, in order for this JSM to be loaded from frame script and other kind of scopes, like shell.js,
we have to keep both ways of exporting symbols. So now, we would have to do:
  let EXPORTED_SYMBOLS = this.EXPORTED_SYMBOLS = ["DebuggerServer"];
  let DebuggerServer = this.DebuggerServer = ...;

Kyle, any idea/thoughts?

Comment 58

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

How have you been reproducing this, Alexandre? I've had a really hard time seeing it, and it would help a lot to have a consistent STR (ideally for something I can run on desktop).

Comment 59

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: b2g-fix

This patch sets the default for loadFrameScript on b2g so that we load into the global scope. It would be much nicer to make everything work the same on desktop and b2g, but I'd rather take this patch than back out everything.

Alexandre said that he sees the exception in bug 951609's title when the phone starts up. I'll see if I can reproduce that somehow.

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8349598 [details] [diff] [review]
b2g-fix

I assume you didn't actually test this, since aArgc is about optional args

Comment 61

[Gregor Wagner [:gwagner]]

This has caused a 50% intermittent travis failure and around 80% intermittent hang when tested locally with a debug gecko. It causes a hang towards the end of the test.

You can run the test from your gaia folder with: NPM_REGISTRY=http://registry.npmjs.org DEBUG=b2g-desktop ./bin/gaia-marionette  ./apps/email/test/marionette/notification_set_interval_test.js --host marionette-b2gdesktop-host --reporter Spec

Note that you need to run 'make test-integration' first and if you want to run with your custom gecko version you have to overwrite the version that gets downloaded into gaia/b2g

Comment 62

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think I know what the problem is here. I'll get a patch up today.

Comment 63

[Gregor Wagner [:gwagner]]

Some debug output that shows the error: https://pastebin.mozilla.org/3810994

It seems 'ReferenceError: Services is not defined' causes the hang. It's not present without this patch.

Comment 64

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Somehow I missed that we landed this ... mrbkap is aware of some sort of pointer replay issue with JSScript*s that we should fix.

Comment 65

[Gregor Wagner [:gwagner]]

(In reply to Bill McCloskey (:billm) from comment #62)
> I think I know what the problem is here. I'll get a patch up today.

Great, thanks!
But please back out if there is no fix in sight in the next few hours.
We are currently in the process of unhiding all those tests on tbpl so that we don't get into this situation any more. But we can't unhide with a 50% intermittent rate :)

Comment 66

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I backed this out because evilpie and I discovered a problem with it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ac2609163d

We have some code in a content script that looks like this:

let WebProgressListener = {
  init: function() {
    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
                              .getInterface(Ci.nsIWebProgress);
    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_ALL);
  },
  ...
  onLocationChange: ...
};
WebProgressListener.init();

With the patch in this bug, all this code runs inside of (function() { ... })(). The problem is that WebProgressListener is now unreachable: it's no longer a property of the global, and the reference created by addProgressListener is weak. We put some printfs in the nsDocLoader.cpp code, and we can see the code here getting triggered:
https://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#1365

I'm not really sure what to do about this problem. Kyle, it seems like the same problem should happen with the reuseGlobals patch since it uses a similar scoping mechanism. Have you run into this? We could, of course, just say |this.WebProgressListener = WebProgressListener;|, but I don't like how easy it is to create subtle bugs this way. It would be nice if all the top-level variables defined by content scripts stayed rooted, but in a different scope. I don't know what the best way to do that is though.

Comment 67

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I wonder if we could create a new JSObject for each content script. Then we could compile the script code with that object as the scope chain and use JS_ExecuteScript to run it. Then we could attach the given JSObject to the global using some mangled name or something.

Comment 68

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I talked to Luke and it sounds like we can do something like comment 67. I'll need to make a variant of JS_ExecuteScript that allows you to provide a scopeObject, a varObject, and a thisObject. That way, we can make the script-specific object be the scopeObject and the varObject, and the global can be the thisObject.

I'm not sure what we should do about the reuseGlobal stuff. This is a tricky bug that can cause odd behavior that's difficult to reproduce. On the other hand, if you're not using weak references for anything, there should be no problem.

Comment 69

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I suspect that in practice the scope object for the function generally gets entrained, which is why we haven't seen this before.

Comment 70

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I may not get to this for a little while, so I'm assigning it to myself so I don't forget it.

Comment 71

[Dan Minor [:dminor]]

This ended up causing Bug 957237 in the 'steeplechase' WebRTC testing framework :ted and I have been working on. The tests began timing out when this landed and started working again when it was backed out.

Comment 72

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 73

[Tom S [:evilpie]]

I have a pretty good idea on how to fix this, but I am not sure yet if it's going to be correct. My idea is to reuse the strict-indirect eval machinery and just keep the scope object for the eval context around.

Comment 74

[Tom S [:evilpie]]

Attachment: WIP

Comment 75

[Tom S [:evilpie]]

The next step would be to add a "nsAutoTArray<JS::Heap<JSObject*>, 2> mAnonymousGlobalScopes;" member to "nsFrameScriptExecutor", append the scope objects to it and also trace it with the CC macros. But I am not sure I can figure out how to do this.

Comment 76

[Tom S [:evilpie]]

Attachment: wip v2

In theory this should work now, but I haven't tested yet if it actually prevents the progess-listener from disappearing.

Comment 77

[Tom S [:evilpie]]

Attachment: wip v2

Last WIP didn't compile.

Comment 78

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: testcase

This is a testcase that fails when I comment out the first line in nsFrameMessageManager::LoadFrameScript and succeeds otherwise. It also seems to work with Tom's patch.

Comment 79

[Tom S [:evilpie]]

Attachment: v1

Jan: Can you please take a look at the JS bits? What about that assert?
Blake: Can you please review the browser bits.

Comment 80

[Tom S [:evilpie]]

This forces strict mode for content-scripts, so this might turn out to be a problem. I also just pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=e65745732c51

Comment 81

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8439492 [details] [diff] [review]
v1

Tryserver looks very orange, so clearing review request.
MessageManager/TabChild stuff looks ok though.

>   JS::Rooted<JSObject*> global(cx, mGlobal->GetJSObject());
>   if (global) {
>-    JSAutoCompartment ac(cx, global);
>     bool ok = true;
>-    if (funobj) {
>-      JS::Rooted<JSObject*> method(cx, JS_CloneFunctionObject(cx, funobj, global));
>-      if (!method) {
>-        return;
>+    {
>+      JSAutoRequest request(cx);
Do you need this? AutoSafeJSContext should have pushed a request thing to stack.

Comment 82

[Tom S [:evilpie]]

I think I have a good handle on the test problem. I fixed most of them now and a new try push looks like this: https://tbpl.mozilla.org/?tree=Try&rev=afef514a272b. I missed bc1 before, but fixed it now locally. The failure in a11y in oth isn't obvious to me yet. At the time of writing b2g isn't done yet, but when the results are too bad I would probably just not enable this patch there.

Comment 83

[Jan de Mooij [:jandem]]

Comment on attachment 8439492 [details] [diff] [review]
v1

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

Looks pretty good, two questions:

(1) Could we add a shell function to expose this, maybe?
(2) Baseline won't return the scope object right? It'd be good to have a shell function to test these cases; it seems like we could just abort Baseline compilation for now until we know everything is stable and working as expected.

::: js/src/builtin/Eval.cpp
@@ +453,5 @@
> +        return false;
> +    }
> +
> +    if (!rval.isObject())
> +        return false;

When will this happen?

::: js/src/vm/Interpreter.cpp
@@ +3447,5 @@
>  
> +
> +    if (activation.entryFrame()->returnScopeObject()) {
> +        state.setReturnValue(ObjectValue(*activation.entryFrame()->scopeChain()));
> +    }

Nit: no {}, you're touching too much Gecko code ;)

::: js/src/vm/Stack-inl.h
@@ +924,5 @@
>      } else {
>          regs_ = state.asGenerator()->gen()->regs;
>      }
>  
> +    //JS_ASSERT_IF(entryFrame_->isEvalFrame(), state_.script()->isActiveEval());

You could change this to something like:

JS_ASSERT_IF(entryFrame_->isEvalFrame(),
             state_.asExecute()->type() == EXECUTE_ANONYMOUS_GLOBAL_EVAL ||
             state_.script()->isActiveEval());

::: js/src/vm/Stack.h
@@ +862,5 @@
>          return !!(flags_ & (GLOBAL | EVAL));
>      }
>  
>      /*
> +     * Set if we want to have

Nit: finish comment

Comment 84

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix-a11y

I think this should fix the a11y problems. Trevor, this patch tries to run each content script in a separate "scope object", which is different from the global. Previously, all content scripts for a given tab were run in the same scope. Content scripts still share a global, which they can access using |this| at the top level.

The a11y code here passes |this| (the global) to EventManager and then tries to use it to access a top-level variable, contentControl. However, that variable is now part of the per-script scope, not the global. To fix this, I added a second argument to EventManager for the contentControl.

Tom, there was still one a11y assertion you were getting in your try run that I can't explain. I'm hoping it will go away when the other problems are fixed.

Comment 85

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks so much for working on this Tom! I did two try runs:

- Just mochitest-other on desktop platforms to make sure the a11y stuff is fixed:
https://tbpl.mozilla.org/?tree=Try&rev=cb6cfcb6b8a7

- All tests on the mobile platforms:
https://tbpl.mozilla.org/?tree=Try&rev=646176109290

Tom, I'm a little worried about forcing strict mode in content scripts. It means that add-on authors will have a really hard time using third-party code in their content scripts, since it might not pass strict mode. Is there anything we can do about this? I don't understand the comment explaining why we need strict mode.

Also, I forgot whether you verified that code inside functions will be Ion-compiled. Could you please make sure?

Comment 86

[Eitan Isaacson [:eeejay]]

Comment on attachment 8443831 [details] [diff] [review]
fix-a11y

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

lgtm

Comment 87

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The b2g push had some issues. This is an attempt to fix some of them:
https://tbpl.mozilla.org/?tree=Try&rev=f4113e3af1d2

Also, I talked to Tom about the strict thing. I guess we need strict mode in order to get a separate scope for eval. However, it would be nice if we could get a separate scope without needing to enable the rest of strict mode. Do you think that would be difficult, Jan?

Comment 88

[Jan de Mooij [:jandem]]

(In reply to Bill McCloskey (:billm) from comment #87)
> Also, I talked to Tom about the strict thing. I guess we need strict mode in
> order to get a separate scope for eval. However, it would be nice if we
> could get a separate scope without needing to enable the rest of strict
> mode. Do you think that would be difficult, Jan?

It's not trivial but it seems possible with some work... In InterpreterFrame::prologue, we push a CallObject on the scope chain for the strict-eval case. It seems like you could check the RETURN_SCOPE_OBJECT InterpreterFrame flag the patch adds, and do the scope pushing in that case as well. Same for InterpreterFrame::epilogue. If we make sure these scripts don't enter the JITs (see my review comment), they shouldn't require any changes.

Then there's some related code in TryConvertFreeName in BytecodeEmitter.cpp (grep for |strict|), to make sure we don't emit *GNAME ops in this case. ScopeIter::settle() will probably also have to be changed. There are likely other places...

You should probably talk to Jason or Luke about this as well. And Nathan, an intern, is also working on scope chain stuff for bug 589199. It looks a bit related.

Comment 89

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix-data-url

This patch appears to fix b2g for me. The code in the content script was not strict mode-safe. I also cleaned it up a little.

Comment 90

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: default-var-obj

This patch is adapted from the one Tom wrote. It adds a new eval function that creates a new scope, evaluates a script in that scope, and returns the scope. It sets |this| to the global.

The main problem I ran into is that JSOP_BINDNAME always uses the global if no binding is found. That means that unqualified variables would go on the global and not the new scope. To fix that problem, I added a new DEFAULT_VAROBJ flag on objects, similar to VAROBJ. VAROBJ is set on any object that |var| can add things to: globals, CallObjects, etc. DEFAULT_VAROBJ is set only on objects where unqualified names should be added. Normally this is just the global, but in this patch it's also the new scope.

Tom's original patch dealt with this problem by always requiring strict mode, where unqualified variables are an error. I'm afraid that would be too limiting for content scripts that want to use third-party code.

Comment 91

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: anonymous-scope

This is pretty much Tom's patch with the JS parts factored out into a separate patch.

Comment 92

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: default-var-obj v2

Whoops. I didn't test the changes to JSOP_IMPLICITTHIS. I guess we still want that to use the global.

Comment 93

[Luke Wagner [:luke]]

Comment on attachment 8454178 [details] [diff] [review]
default-var-obj v2

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

::: js/src/vm/Shape.h
@@ +310,5 @@
>          ITERATED_SINGLETON  =  0x200,
>          NEW_TYPE_UNKNOWN    =  0x400,
>          UNCACHEABLE_PROTO   =  0x800,
>          HAD_ELEMENTS_ACCESS = 0x1000,
> +        DEFAULT_VAROBJ      = 0x2000,

What about renaming VAROBJ to QUALIFIED_VAROBJ and DEFAULT_VAROBJ to UNQUALIFIED_VAROBJ?  And then could you (be the first person in the history of SM to) document the meaning of VAROBJ (at which point you could explain 'qualified' vs 'unqualified')?

Comment 94

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/666a2522440a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc6ba5dccac

Comment 95

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/666a2522440a
https://hg.mozilla.org/mozilla-central/rev/6cc6ba5dccac

Comment 96

[Wladimir Palant]

Is target milestone wrong here? If I see it correctly, it landed in mozilla29 originally but was backed out. I can reproduce this issue in Firefox 31 and from the look of it the fix only landed in mozilla33.

Comment 97

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Yeah, the relanding happened during Firefox 33's time in Nightly.