Closed Bug 1276133 Opened 3 years ago Closed 3 years ago

Stop using GetScriptContextFromJSContext in nsGlobalWindow::OpenInternal

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

We should be able to use the entry global here instead, I would think, and check whether it's one of our inners.  That would be equivalent to what we do now, I think.
Note that the change in the condition is ok because we have the invariant that
aCalledNoScript != bool(aJSCallerContext), though we never actually clearly
assert that in this method (we instead assert that at least one has to be false,
but it turns out they can't _both_ be false either.
Attachment #8757131 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(In reply to Boris Zbarsky [:bz] from comment #2)
> Created attachment 8757132 [details] [diff] [review]
> part 2.  Remove the now-unused aJSCallerContext argument of
> nsGlobalWindow::OpenInternal

Patch is empty.
Attachment #8757132 - Attachment is obsolete: true
Attachment #8757132 - Flags: review?(bugs)
Blocks: 1276276
Comment on attachment 8757131 [details] [diff] [review]
part 1.  Stop using GetDefaultScopeFromJSContext in nsGlobalWindow::OpenInternal.  Instead, use the entry global's outer window (if the entry global is a window), which should correspond to the stack-top JSContext in all web-visible cases right now

Yeah, looks like aJSCallerContext == !aCalledNoScript
Attachment #8757131 - Flags: review?(bugs) → review+
Comment on attachment 8757131 [details] [diff] [review]
part 1.  Stop using GetDefaultScopeFromJSContext in nsGlobalWindow::OpenInternal.  Instead, use the entry global's outer window (if the entry global is a window), which should correspond to the stack-top JSContext in all web-visible cases right now

hmm, or, I couldn't convince this is the same for OpenDialogOuter case, but
that is chrome only, so checkForPopup is false and this code doesn't run at all, so fine.
Attachment #8757337 - Flags: review?(bugs) → review+
> hmm, or, I couldn't convince this is the same for OpenDialogOuter case

In the OpenDialogOuter, we pass false for aCalledNoScript.  So the only question is whether aCx can be null there.  The only caller of OpenDialogOuter is OpenDialog, the version with a JSContext argument.  The only caller of that is binding code, so we're definitely going to have a JSContext there.
(In reply to Boris Zbarsky [:bz] from comment #7)
> In the OpenDialogOuter, we pass false for aCalledNoScript.  So the only
> question is whether aCx can be null there.  The only caller of
> OpenDialogOuter is OpenDialog, the version with a JSContext argument.  The
> only caller of that is binding code, so we're definitely going to have a
> JSContext there.
I was wondering whether mContext == GetScriptContextFromJSContext(aJSCallerContext) comparison is the same as entryWindow->GetOuterWindow() == this->AsOuter(), not whether cx is passed or anything.
Ah.  That should be the case as long as people aren't messing up AutoEntryScript vs AutoJSAPI...
https://hg.mozilla.org/mozilla-central/rev/6adda4788ea8
https://hg.mozilla.org/mozilla-central/rev/ff41a86fc3b3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.