Status

()

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: bobbyholley, Assigned: bzbarsky)

Tracking

(Depends on 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(11 attachments)

6.47 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
5.24 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
8.64 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
2.32 KB, patch
bobbyholley
: review+
khuey
: review+
Details | Diff | Splinter Review
3.16 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
3.92 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
11.09 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
5.59 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
5.95 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
11.32 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
14.33 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
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.
Depends on: 767942
More generally, I think this title better reflects what needs to happen here. Removing GetDynamicScriptContext will hopefully be the main piece here.
No longer depends on: 767942
Summary: Remove GetDynamicScriptContext → Make DOM code context-agnostic
Summary: Make DOM code context-agnostic → Make DOM code JSContext-agnostic
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.
Blocks: 796938
No longer blocks: 796938
Depends on: 796938
No longer blocks: CVE-2012-4201
Depends on: 860429
Depends on: 880330
Depends on: 881811
No longer blocks: 754202
Blocks: 896535
Depends on: 899367
Depends on: 901364
Blocks: 650361
Depends on: 944011
Updating the title to better reflect the end goal here.
Summary: Make DOM code JSContext-agnostic → rm JSContext stack
No longer depends on: 881811
Depends on: 979730
Depends on: 981218
Depends on: 951992
No longer depends on: 796938
Blocks: 604813
No longer depends on: 951992
Depends on: 951992
Assignee: bobbyholley → nobody
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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; }
Attachment #8764820 - Flags: review?(bobbyholley) → review+
Attachment #8764821 - Flags: review?(bobbyholley) → review+
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.
Attachment #8764822 - Flags: review?(bobbyholley) → review+
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?
Attachment #8764823 - Flags: review?(bobbyholley) → review+
Attachment #8764824 - Flags: review?(bobbyholley) → review+
Attachment #8764825 - Flags: review?(bobbyholley) → review+
Attachment #8764826 - Flags: review?(bobbyholley) → review+
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.
Attachment #8764827 - Flags: review?(bobbyholley) → review+
Attachment #8764828 - Flags: review?(bobbyholley) → review+
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.
Attachment #8764829 - Flags: review?(bobbyholley) → review+
Attachment #8764830 - Flags: review?(bobbyholley) → review+
> 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
You need to log in before you can comment on or make changes to this bug.