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

Could this be the root cause of bug 668471?

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).

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?

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.

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

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 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.

(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?

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 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.

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

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.)

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.

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

I have no idea in which directory to run that command

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 on attachment 549178 [details] [diff] [review] patch Oops, wrong bug.

(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.

(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)

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?

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 :)

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.

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

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

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.

felipe said that he'll fix Fennec code.

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 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 on attachment 555513 [details] [diff] [review] +comments It might be worth factoring out the error reporting into a common function. r=me either way.

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

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?

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

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.

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

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.

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() {...})().

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?

I posted the JS changes to bug 682048.

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

> > 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.

Right, but could you add a comment.

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.

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

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

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

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

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

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 = ...;

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?

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).

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 on attachment 8349598 [details] [diff] [review] b2g-fix I assume you didn't actually test this, since aArgc is about optional args

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

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

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.

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

(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 :)

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.

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.

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.

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

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

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.

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

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.

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.

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

Last WIP didn't compile.

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.

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

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 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.

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 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

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.

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 on attachment 8443831 [details] [diff] [review] fix-a11y Review of attachment 8443831 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

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?

(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.

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.

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.

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

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

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')?

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

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

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.

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