767938 - rm JSContext stack

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Luke Wagner [:luke]]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Bobby Holley (:bholley)]

Updating the title to better reflect the end goal here.

Comment 6

[Jan de Mooij [:jandem]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change ScriptSettingsStackEntry to allow having stack entries that are neither candidates for being entry globals nor candidates for being incumbent globals

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Move control over pushing/popping ScriptSettingsStackEntry instances into subclasses, so we can do the conditional pushing/popping AutoJSAPI will need

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make AutoJSAPI a ScriptSettingsStackEntry

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Put an AutoNoJSAPI on the stack while running events off the event loop

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Stop using the JSContext stack to get the current JSContext

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Get rid of XPConnect's GetCurrentJSContext getter

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove the now-debug-only uses of XPCJSContextStack::Peek and Count()

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove the no-longer-really-needed Debug_SetActiveJSContext

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Move the JSAutoRequest from AutoCxPusher to AutoJSAPI, because we're about to kill off AutoCxPusher

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Remove the now write-only XPCJSContextStack's actual stack of JSContexts and AutoCxPusher

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Move the "safe JS context" to where it belongs: the CycleCollectedJSRuntime

Comment 19

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73c589b7fcf2

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 21

[Bobby Holley (:bholley)]

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 22

[Bobby Holley (:bholley)]

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 23

[Bobby Holley (:bholley)]

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 24

[Bobby Holley (:bholley)]

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 25

[Bobby Holley (:bholley)]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Phil Ringnalda (:philor)]

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