Closed Bug 796938 Opened 8 years ago Closed 6 years ago

Remove usage of GetDynamicScriptContext

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bzbarsky, Assigned: bholley)

References

(Blocks 2 open bugs)

Details

Attachments

(13 files, 1 obsolete file)

3.24 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.44 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.20 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.20 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.03 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.86 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.73 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.31 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.83 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
We should understand this, so we can sort out what needs to go in the script entry point stack.
I'm investigating this now.
Assignee: nobody → bobbyholley+bmo
nsContentUtils.cpp:
-Used for determining whether a given nsJSContext is on the stack, which sets mScriptEvaluting.

nsGlobalWindow.cpp:
-Used to get the dynamic script global
-Used to determine whether |this|'s cx is on the top of the stack.
-Used to get the dynamic script global

nsJSEnvironment.cpp
-Used to get the dynamic script global

nsLocation.cpp
-Used to get the topmost cx on the stack that has a dynamic script global
-Something tricky with GetProcessingScriptTag

nsJSUtils.cpp
-Used to implement GetDynamicScriptGlobal

nsWindowWatcher.cpp
-Used to get a kungFuDeathGrip on a JSContext stored in an RAII struct.
-Used to get the dynamic script global
So at first glance, this looks pretty encouraging. Pretty much everybody just wants to know the dynamic script global, which is probably what we're going to end up storing on the script entry point stack.

So I think the first step here is to remove all uses of GetDynamicScriptContext and replace them with calls to GetDynamicScriptGlobal. Then, we can re-implement GetDynamicScriptGlobal with the script entry point stack, and kill context pushing altogether.

Morphing this bug appropriately.
Depends on: 767938
Summary: Figure out what GetDynamicScriptContext and GetDynamicScriptGlobal callers are really trying to do → Remove usage of GetDynamicScriptContext
Blocks: 767938
No longer depends on: 767938
So, it looks like the wackiness in nsLocation goes away with the patch in bug 754029. So we just need to get that landed.
Depends on: 754029
Ah crap. Looks like there's _also_ GetScriptContextFromJSContext, which is a whole other can of worms.
Depends on: 824864
This was my original patch to remove the API. Since bug 754029 was backed out,
this no longer compiles for me with my local patch stack, so I'm posting it here
before reverting it.
Depends on: 841312
Depends on: 951991
No longer blocks: 767938
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 951992
Actually, I think this is a good chunk size to exist on its own.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 951992
No longer depends on: 754029
Status: REOPENED → NEW
OS: Mac OS X → All
Hardware: x86 → All
Attachment #713838 - Attachment is obsolete: true
We leave this flag on the script context for now - we can move it somewhere
else later.
Attachment #8474303 - Flags: review?(bugs)
Comment on attachment 8474291 [details] [diff] [review]
Part 1 - Switch nsGlobalWindow::Focus to GetEntryGlobal and eliminate nsContentUtils::GetWindowFromCaller. v1

(I wonder where the word 'Dynamic' comes to GetDynamicScriptGlobal. Nothing dynamic there, based on the implementation.)
Attachment #8474291 - Flags: review?(bugs) → review+
Comment on attachment 8474292 [details] [diff] [review]
Part 2 - Switch nsHTMLDocument::Open and XMLDocument::Load to a new GetEntryDocument API and remove nsContentUtils::GetDocumentFromContext. v1

Hmm, I wish Get*Global and GetEntryDocument() weren't global
methods. Making them static methods of some class would make the 
code easier to read.
File a followup for that.
Attachment #8474292 - Flags: review?(bugs) → review+
Attachment #8474293 - Flags: review?(bugs) → review+
Attachment #8474294 - Flags: review?(bugs) → review+
Attachment #8474295 - Flags: review?(bugs) → review+
Attachment #8474296 - Flags: review?(bugs) → review+
Comment on attachment 8474297 [details] [diff] [review]
Part 7 - Switch nsLocation::GetSourceBaseURL to GetEntryDocument. v1

>-  if (!sgo && GetDocShell()) {
>-    sgo = GetDocShell()->GetScriptGlobalObject();
>+  if (!doc) {
>+    nsCOMPtr<nsPIDOMWindow> docShellWin;
>+    if (GetDocShell()) {
>+      docShellWin = do_QueryInterface(GetDocShell()->GetScriptGlobalObject());
>+    }
>+    if (docShellWin) {
>+      doc = docShellWin->GetDoc();
>+    }
A bit oddly written.
Why not
  if (!doc && GetDocShell()) {
    nsCOMPtr<nsPIDOMWindow> docShellWin = 
      do_QueryInterface(GetDocShell()->GetScriptGlobalObject());
    if (docShellWin) {
      doc = docShellWin->GetDoc();
    }
  }
Attachment #8474297 - Flags: review?(bugs) → review+
Attachment #8474298 - Flags: review?(bugs) → review+
Comment on attachment 8474299 [details] [diff] [review]
Part 9 - Use GetEntryGlobal in nsGlobalWindow::FireAbuseEvents. v1

We should use incumbent global here, I think, but since the old code is using
dynamic script context.
Perhaps file a followup?
Attachment #8474299 - Flags: review?(bugs) → review+
Comment on attachment 8474300 [details] [diff] [review]
Part 10 - Use GetEntryGlobal in NS_ScriptErrorReporter. v1

Why can we replace getting global from cx here with GetEntryGlobal()?
Doesn't look right to me, at least from API perspective.

We're given a cx, and should use that to get the global.
Attachment #8474300 - Flags: review?(bugs) → review-
Attachment #8474302 - Flags: review?(bugs) → review+
Attachment #8474303 - Flags: review?(bugs) → review+
Comment on attachment 8474304 [details] [diff] [review]
Part 13 - Remove GetDynamicScriptContext API. v1

!
Attachment #8474304 - Flags: review?(bugs) → review+
> I wonder where the word 'Dynamic' comes to GetDynamicScriptGlobal

Dynamic as in "whatever global we happened to enter script on" as opposed to "static" which is "the global of the currently running script".  Think dynamic vs static scoping.
(In reply to Olli Pettay [:smaug] from comment #31)
> Comment on attachment 8474300 [details] [diff] [review]
> Part 10 - Use GetEntryGlobal in NS_ScriptErrorReporter. v1
> 
> Why can we replace getting global from cx here with GetEntryGlobal()?
> Doesn't look right to me, at least from API perspective.

Can you explain what you mean here? Are you suggesting that we use GetCurrentGlobal() instead of GetEntryGlobal()? I think that would break things.

 
> We're given a cx, and should use that to get the global.

From a practical perspective, the cx we have here is going to be stack-top, and therefore the same one used by the ScriptSettings.cpp machinery, so it doesn't matter whether we pass it explicitly or not.

From an API perspective, the whole point of these patches is to stop getting global objects from particular JSContexts, so that we can switch to just using one JSContext for Gecko and (eventually) remove JSContexts altogether.

Note that in bug 981198, we're going to move the error reporter off the JSContext entirely.
Flags: needinfo?(bugs)
What guarantees the cx is from stack-top?

NS_ScriptErrorReporter eventually probably take some other param than cx. Global JS object perhaps.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #35)
> What guarantees the cx is from stack-top?

We have extremely strong assertions (both inside and outside the JS engine) that we only ever operate with the stack-top cx.
 
> NS_ScriptErrorReporter eventually probably take some other param than cx.
> Global JS object perhaps.

My plan is to eliminate it in its current form with bug 981187, and have AutoJSAPI take care of reporting or propagating exceptions. The eventual error reporting function will probably accept a global object, but we can't do that as long as the signature is still JSErrorReporter.

Can you be more specific as to what you want to see in part 10?
Comment on attachment 8474300 [details] [diff] [review]
Part 10 - Use GetEntryGlobal in NS_ScriptErrorReporter. v1

Ok, can you at least assert that cx is the stack-top ?
Attachment #8474300 - Flags: review- → review+
Blocks: 1055729
Note that there was a green try push in comment 13. The only substantive code change since then is the addition of an assertion in NS_ScriptErrorReporter.
Blocks: 810808
Depends on: 1057339
Depends on: 1057405
Depends on: 1063271
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.