In order to stop pushing contexts everywhere (which we need for bug 754202), we need to remove the places where we base security checks on the JS context itself (rather than the compartment). This is the big one.

More generally, I think this title better reflects what needs to happen here. Removing GetDynamicScriptContext will hopefully be the main piece here.

This sounds like a most righteous goal. Would this entail (or make easy) removing cx->globalObject (bug 604813)?

(In reply to Luke Wagner [:luke] from comment #2) > This sounds like a most righteous goal. Would this entail (or make easy) > removing cx->globalObject (bug 604813)? Yes.

Note that after talking with bz, this turns out to be a pretty big task. I still might want to tackle it, but I want to land bug 754202 on its own first.

Updating the title to better reflect the end goal here.

bz/bholley, can we close this, or is there more XPCJSContextStack stuff to refactor/remove?

There's more. Right now we still maintain a stack of JSContext* pointers. At this point it's logically equivalent to a stack of booleans (nullptr <=> false, non-null <=> true). Bobby and I were just discussing whether we want to go ahead and make that change, or try to nix the stack more thoroughly.

Comment on attachment 8764826 [details] [diff] [review] part 7. Remove the now-debug-only uses of XPCJSContextStack::Peek and Count() Review of attachment 8764826 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/ScriptSettings.cpp @@ +66,4 @@ > } > entry = entry->mOlder; > } > + return nullptr; trailing ws

Comment on attachment 8764820 [details] [diff] [review] part 1. Change ScriptSettingsStackEntry to allow having stack entries that are neither candidates for being entry globals nor candidates for being incumbent globals Review of attachment 8764820 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that. ::: dom/base/ScriptSettings.cpp @@ +59,5 @@ > > static nsIGlobalObject* IncumbentGlobal() { > ScriptSettingsStackEntry *entry = Top(); > + while (entry) { > + if (entry->mCandidateType != ScriptSettingsStackEntry::eNotCandidate) { Then here we would just do if (entry->IsIncumbentCandidate()) @@ +73,2 @@ > while (entry) { > + if (entry->mCandidateType == ScriptSettingsStackEntry::eEntryCandidate) { And here we would do entry->IsEntryCandidate() ::: dom/base/ScriptSettings.h @@ +163,5 @@ > + enum CandidateType { > + eEntryCandidate, > + eIncumbentCandidate, > + eNotCandidate > + }; If we're going to introduce an enum anyway, I'd rather remove the multiplexing we do with NoJSAPI and add some accessors instead. Basically: enum EntryType { eEntryScript, eIncumbentScript, eJSAPI, eNoJSAPI } And then we can define our state tables crisply in one place: bool NoJSAPI() { return mType == eNoJSAPI; } bool IsEntryCandidate() { return mType == eEntryScript || mType == eNoJSAPI; } bool IsIncumbentCandidate() { return mType != eJSAPI; }

Comment on attachment 8764822 [details] [diff] [review] part 3. Make AutoJSAPI a ScriptSettingsStackEntry Review of attachment 8764822 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/ScriptSettings.cpp @@ +135,5 @@ > : mGlobalObject(aGlobal) > , mCandidateType(aCandidateType) > , mOlder(nullptr) > { > + MOZ_ASSERT_IF(aCandidateType != eNotCandidate, mGlobalObject); Given this, can we relax it a tiny bit more and then get rid of the AutoNoJSAPI-helping constructor below? @@ +141,3 @@ > "Must have an actual JS global for the duration on the stack"); > + MOZ_ASSERT(!mGlobalObject || > + JS_IsGlobalObject(mGlobalObject->GetGlobalJSObject()), Might as well make these MOZ_ASSERT_IF as well.

Comment on attachment 8764823 [details] [diff] [review] part 4. Put an AutoNoJSAPI on the stack while running events off the event loop Review of attachment 8764823 [details] [diff] [review]: ----------------------------------------------------------------- Seems like this patch should simultaneously stop pushing/popping in {Before,After}ProcessNextTask right?

Comment on attachment 8764827 [details] [diff] [review] part 8. Remove the no-longer-really-needed Debug_SetActiveJSContext Review of attachment 8764827 [details] [diff] [review]: ----------------------------------------------------------------- Glad we finally don't need this anymore.

Comment on attachment 8764829 [details] [diff] [review] part 10. Remove the now write-only XPCJSContextStack's actual stack of JSContexts and AutoCxPusher Review of attachment 8764829 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/ScriptSettings.cpp @@ +346,1 @@ > mAutoRequest.emplace(mCx); Add a comment that there's no particular reason we only do a request on the main thread, in case somebody wants to get rid of aIsMainThread.

> I'd rather remove the multiplexing we do with NoJSAPI and add some accessors instead. Done. > Given this, can we relax it a tiny bit more and then get rid of the AutoNoJSAPI-helping constructor below? Yes, done. > Might as well make these MOZ_ASSERT_IF as well. Not done, since MOZ_ASSERT_IF can't have a descriptive string. > Seems like this patch should simultaneously stop pushing/popping in {Before,After}ProcessNextTask right? OK, done. > trailing ws That code is gone because in the new type world it needs to look different. > Add a comment that there's no particular reason we only do a request on the main thread Done.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/48e7f620a02e part 1. Change ScriptSettingsStackEntry to allow having stack entries that are neither candidates for being entry globals nor candidates for being incumbent globals. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/b27e490c048a part 2. Move control over pushing/popping ScriptSettingsStackEntry instances into subclasses, so we can do the conditional pushing/popping AutoJSAPI will need. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/c3369c0fd825 part 3. Make AutoJSAPI a ScriptSettingsStackEntry. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/e6cdf461b61a part 4. Put an AutoNoJSAPI on the stack while running events off the event loop. r=bholley,khuey https://hg.mozilla.org/integration/mozilla-inbound/rev/e636c7186286 part 5. Stop using the JSContext stack to get the current JSContext. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/43e691e50e19 part 6. Get rid of XPConnect's GetCurrentJSContext getter. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/e13615311daf part 7. Remove the now-debug-only uses of XPCJSContextStack::Peek and Count(). r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/60ddb06c3c50 part 8. Remove the no-longer-really-needed Debug_SetActiveJSContext. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc55c9bf8ed part 9. Move the JSAutoRequest from AutoCxPusher to AutoJSAPI, because we're about to kill off AutoCxPusher. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/44ad8744579e part 10. Remove the now write-only XPCJSContextStack's actual stack of JSContexts and AutoCxPusher. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0cd542e1e9 part 11. Move the "safe JS context" to where it belongs: the CycleCollectedJSRuntime. r=bholley

https://hg.mozilla.org/mozilla-central/rev/48e7f620a02e https://hg.mozilla.org/mozilla-central/rev/b27e490c048a https://hg.mozilla.org/mozilla-central/rev/c3369c0fd825 https://hg.mozilla.org/mozilla-central/rev/e6cdf461b61a https://hg.mozilla.org/mozilla-central/rev/e636c7186286 https://hg.mozilla.org/mozilla-central/rev/43e691e50e19 https://hg.mozilla.org/mozilla-central/rev/e13615311daf https://hg.mozilla.org/mozilla-central/rev/60ddb06c3c50 https://hg.mozilla.org/mozilla-central/rev/cbc55c9bf8ed https://hg.mozilla.org/mozilla-central/rev/44ad8744579e https://hg.mozilla.org/mozilla-central/rev/1a0cd542e1e9