951991 - Route all cx pushing through AutoJSAPI and Implement GetEntryGlobal

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

From bug 937317, we currently have a partial implementation of the script settings stack [1].

The letter of the spec indicates that the script settings stack should be munged at every function call, and that the items pushed onto it should in some cases (relatively rarely) be marked as candidate entry points. The incumbent is the topmost item. The entry point is the topmost item that is marked as a candidate entry point.

Implementing this verbatim would obviously be super inefficient. So we cheat, and only munge the explicit stack in two cases:
(1) When the thing being pushed would be a candidate entry point (since the JS engine can't tell us anything useful about entry points).
(2) When examining the innermost JSScript* on the JSRuntime is not guaranteed to match the incumbent script.

We do the munging for (1) with AutoEntryScript, and the munging for (2) with AutoIncumbentScript.

Because our strategy for computing the incumbent relies mostly on the pre-existing information in the JS engine, the number of places where we need to use AutoIncumbentScript are quite few. We've (hopefully) got those covered at this point, so we can expose GetIncumbentGlobal() and trust that it returns the right thing in all cases.

However, exposing GetEntryGlobal() requires us to use AutoEntryScript in all the places where we need a candidate entry point. Currently, we more or less track the entry point with the JSContext stack. JSContexts are going away, so this needs to change.

So the main thing we need to do here is to go through all of the places where we explicitly push a JSContext (via nsCxPusher, AutoCxPusher, and AutoPushJSContext), and replace them. They fall into 4 categories:
(A) Entry points specified in the spec.
(B) Entry points not in the spec, but that should be added to the spec.
(C) Entry points that should not be in the spec, but that we need because Gecko's architecture introduces complications that don't exist in the spec.
(D) Places where we push a JSContext for no good reason at all.

For each of the cases that fall into (A-C), we should switch to an AutoEntryScript (which, for now, also pushes a JSContext, so that it preserves the old behavior during the transition period). For (B) we should file spec bugs. For (D), we should remove the cx push.

This work is likely too much to land all at once, and should land iteratively in dependent bugs. Once it's all done, implementing GetEntryGlobal is just ~10 lines of code.

[1] http://www.whatwg.org/specs/web-apps/current-work/#calling-scripts

Comment 1

[Andrew McCreight [:mccr8]]

Is this on your radar, Bobby?  It is blocking some other stuff.

Comment 2

[Bobby Holley (:bholley)]

Yeah, Bob is working on it.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Bug 951991: Design decisions that might be relevant in general.

(In reply to Bobby Holley (:bholley) from comment #2)
> Yeah, Bob is working on it.

Well, I am now ... sorry about the delay.
I got myself tied in knots re-writing some of the iframe sandbox tests that have been producing oranges fairly frequently since September.
The week's holiday in between probably didn't help (my progress, not me :-) ).

I didn't realise that this was reasonably urgent, I thought it was more of a background task.

Anyway, I've got my patches for the re-writes uploaded, so I'll see if I can spend much more time on it and start to make some progress.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #3)
> > Yeah, Bob is working on it.
> 
> Well, I am now ... sorry about the delay.

No worries - PTO is sacred!

> I didn't realise that this was reasonably urgent, I thought it was more of a
> background task.

It's somewhere in between. We need to get it done, but it's still worth being careful and incremental about it, and we don't anyone to burn out trying to do it all at once. So no need to drop everything else, but just try to keep this moving. :-)

> Anyway, I've got my patches for the re-writes uploaded, so I'll see if I can
> spend much more time on it and start to make some progress.

Sounds great! I r+ed the first one already.

Comment 5

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=de771d4b2d64

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=49842c58176e

Comment 7

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ded6b3a40a0b

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Assert against calling GetCurrentInnerWindow on an inner. v1

The current convention is that this returns null when invoked on an inner, which
callers may or may not handle correctly. But when we start using GetEntryGlobal,
we'll end up with a lot of inners where we used to get outers, so we should get
strict about this now.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement GetEntryGlobal. v1

Comment 10

[Bob Owen (:bobowen)]

Comment on attachment 8474290 [details] [diff] [review]
Part 2 - Implement GetEntryGlobal. v1

Review of attachment 8474290 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ScriptSettings.cpp
@@ +129,4 @@
>  }
>  
>  // Note: When we're ready to expose it, GetEntryGlobal will look similar to
>  // GetIncumbentGlobal below.

This note can go now.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8474289 [details] [diff] [review]
Part 1 - Assert against calling GetCurrentInnerWindow on an inner. v1

>-  if (inner) {
>-    inner->CleanUp();
>+  if (IsOuterWindow()) {
>+    nsGlobalWindow *inner = GetCurrentInnerWindowInternal();
* goes with the type

>   bool IsLoadingOrRunningTimeout() const
>   {
>-    const nsPIDOMWindow *win = GetCurrentInnerWindow();
>-
>-    if (!win) {
>-      win = this;
>-    }
>-
>+    const nsPIDOMWindow *win = IsInnerWindow() ? this : GetCurrentInnerWindow();
ditto

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8474290 [details] [diff] [review]
Part 2 - Implement GetEntryGlobal. v1

>-// This mostly gets the entry global, but doesn't entirely match the spec in
>-// certain edge cases. It's good enough for some purposes, but not others. If
>-// you want to call this function, ping bholley and describe your use-case.
>-nsIGlobalObject* BrokenGetEntryGlobal();
>+// Returns the global associated with the top-most Candidate Entry Point on
>+// the Script Settings Stack. See HTML5. This may be null.
>+nsIGlobalObject* GetEntryGlobal();
> 
>-// Note: We don't yet expose GetEntryGlobal, because in order for it to be
>-// correct, we first need to replace a bunch of explicit cx pushing in the
>-// browser with AutoEntryScript. But GetIncumbentGlobal is simpler, because it
>-// can mostly be inferred from the JS stack.
>+// Returns the global associated with the top-most entry of the the Script
>+// Settings Stack. See HTML5.
> nsIGlobalObject* GetIncumbentGlobal();
> 


Even if the spec was good at explaining what the difference is between
EntryGlogal and IncumbentGlobal (which it certainly isn't, https://www.w3.org/Bugs/Public/show_bug.cgi?id=26547)
we should add good examples here indicating when one should use which Global.
So, just because miss the documentation here, r-

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I'm expecting to see some JS examples which tell which global is which object in case of 
several globals.
(This is a bit complicated stuff and the spec is pretty much unreadable in its current form so
I can't expect anyone using this API to understand the spec.)

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement GetEntryGlobal. v2

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #10)
> >  // Note: When we're ready to expose it, GetEntryGlobal will look similar to
> >  // GetIncumbentGlobal below.
> 
> This note can go now.

(fixed this locally).

Comment 16

[Bob Owen (:bobowen)]

Comment on attachment 8474874 [details] [diff] [review]
Part 2 - Implement GetEntryGlobal. v2

Review of attachment 8474874 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ScriptSettings.h
@@ +78,5 @@
> +// current compartment. This roughly corresponds with the notion of Realms in
> +// ECMAScript. Note that this concept may be merged with that of the incumbent
> +// global - see [1].
> +//
> +// Suppose some event trigger an event listener in window A, which invokes a

nit: triggers

@@ +86,5 @@
> +//
> +// In general, it's best to use to use the most-closely-associated global
> +// unless the spec says to do otherwise. In 95% of the cases, the global of
> +// the current compartment (GetCurrentGlobal()) is the right thing. The
> +// incumbent global is very similar, and may not need to be a separate concept

Won't we at least always need incumbent global for navigation?
It's sometimes used to get the source browsing context.
I can't see how iframe sandboxing would work if there were no distinction between the incumbent global and the current global.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8474874 [details] [diff] [review]
Part 2 - Implement GetEntryGlobal. v2

>-// This mostly gets the entry global, but doesn't entirely match the spec in
>-// certain edge cases. It's good enough for some purposes, but not others. If
>-// you want to call this function, ping bholley and describe your use-case.
>-nsIGlobalObject* BrokenGetEntryGlobal();
>+// To implement a web-compatible browser, it is often necessary to obtain the
>+// global object that is "associated" with the currently-running code. This
>+// process is made more complicated by the fact that, historically, different
>+// algorithms have operated with different definitions of the "associated"
>+// global.
>+//
>+// HTML5 formalizes this into two concepts: the "incumbent global" and the
>+// "entry global". The incumbent global corresponds to the global of the
>+// current script being executed, whereas the entry global corresponds to the
>+// global of the script where the current JS execution began.
>+//
>+// There is also a potentially-distinct third global that is determined by the
>+// current compartment. This roughly corresponds with the notion of Realms in
>+// ECMAScript. Note that this concept may be merged with that of the incumbent
>+// global - see [1].
>+//
>+// Suppose some event trigger an event listener in window A, which invokes a
>+// scripted function in window B, which invokes the href setter in window C.
What href setter? window object doesn't have such.

It might be good to explain also what global webidl constructor get.
If the listener (from B) calls 'new windowC.XMLHttpRequest()', the constructor
gets C as param (or at least it should since all the code expects that and that is what
pre-webidl code did).


>+// In general, it's best to use to use the most-closely-associated global
>+// unless the spec says to do otherwise. In 95% of the cases, the global of
>+// the current compartment (GetCurrentGlobal()) is the right thing. The
>+// incumbent global is very similar, and may not need to be a separate concept
>+// at all (again, see [1]). The entry global is used for various things like
>+// computing base URIs, mostly for historical reasons.
Ah good, getting rid of the incumbent jargon would be great, but
based on https://www.w3.org/Bugs/Public/show_bug.cgi?id=26603#c1 it sounds like we can't really do it.


>+// Returns the global associated with the top-most Candidate Entry Point on
>+// the Script Settings Stack. See HTML5. This may be null.
>+nsIGlobalObject* GetEntryGlobal();
>+
>+// Returns the global associated with the top-most entry of the the Script
>+// Settings Stack. See HTML5. This may be null.
> nsIGlobalObject* GetIncumbentGlobal();

s/See HTML5/See HTML/

(Hixie is the editor of HTML spec, not HTML5)


Thanks.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

One thing which must be still documented is that whether these methods return inner or outer window.

Comment 19

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=25fff355894f

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6b5285b794ba
https://hg.mozilla.org/mozilla-central/rev/5ed2046aa6c6