Centralize setting the active document #2671

annevk · 2017-05-12T12:56:21Z

annevk · 2017-05-12T12:59:52Z

This fix as-is doesn't work and intentionally doesn't compile. I need input from @wanderview and @jungkees and maybe others as to how to interpret the requirements at step 13 and 14 of https://html.spec.whatwg.org/multipage/browsers.html#initialise-the-document-object. And also why that was only done there and not when creating a new auxiliary browsing context.

annevk · 2017-05-12T13:20:43Z

Note that in this patch that bit is labeled as "XXX".

annevk · 2017-05-12T13:37:26Z

The problematic text was added in #1776, reviewed by myself. Bah.

annevk · 2017-05-12T13:42:53Z

And as far as I can tell the opener bit wasn't discussed in the review and in the design. @jungkees I need your help.

annevk · 2017-05-12T14:14:00Z

source

+   <li>
+    <p>If <var>settingsObject</var>'s <span
+    data-x="concept-environment-active-service-worker">active service worker</span> is non-null,
+    then:</p>


Having studied this block for a bit I really think it's likely wrong. Setting disowning opener doesn't help you much, especially if they already had a reference to your WindowProxy object. I hope someone can tell me whether this is actually implemented. If not, I suggest we remove it and open an issue for it if we need a solution of sorts, as this blocks refactoring navigation.

I would be shocked if we implemented that. Pretty sure we will control a document regardless of the secure status of its opener.

Thanks, would be great if @jungkees could help sort this through in order to figure out what should be done instead.

(Already discussed with @annevk in the issue: #2657 (comment))

The disowning opener bit was a follow-up on w3c/ServiceWorker#890 (comment) (from the SW f2f July 2016.)

Setting disowning opener doesn't help you much, especially if they already had a reference to your WindowProxy object.

So, we'd need to put the disowning step or equivalent step to somewhere before the script is run I guess.

Filed #2687 as follow-up.

domenic · 2017-05-12T18:41:53Z

source

+  steps:</p>
+
+  <ol>
+   <li><p>Let <var>window</var> be <var>document</var>'s <span>relevant global object</span>.


Missing closing tags here and in various other steps of the algorithm

domenic · 2017-05-12T18:44:31Z

source

+  steps:</p>
+
+  <ol>
+   <li><p>Let <var>window</var> be <var>document</var>'s <span>relevant global object</span>.


"relevant global object" needs a data-x; it's not linking

domenic · 2017-05-12T18:49:23Z

source

+   <code>Document</code></span> to <var>document</var>.</p></li>
+
+   <li><p>Set <var>browsingContext</var>'s <span>active document</span> to
+   <var>document</var>.</p></li>


I'd always thought the active document would be a "computed" value, e.g. it would be defined to be browsingContext's WindowProxy's [[Window]]'s associated Document. I now see that wasn't how the previous spec worked, as we always set the active document during history traversal. However, it's still an easy assumption to make, and might even be true.

If it's not true, it would be good to note that near the definition of active document, preferably with an example of when they're different. If it is true, then this step should not be needed, and we can just define it in that "computed" way.

So I think that is true, but I realize there's a bug in document.open() now as it doesn't update [[Window]]. So document.open() should probably invoke these steps too although maybe not as not all of it might make sense? Gah, I wish we stopped doing new globals for document.open() and gave up on that.

Filed #2676. Not entirely sure what to do about that. I guess I need to know what it means for the "execution ready flag", but of course with Chrome and Firefox having extremely divergent designs for document.open() there's no way of really knowing what would be best. Sigh.

I think we should fix document.open() to use these steps, and then turn "active document" into a computed concept. We know there's tech debt there in general (about new globals), so it's OK to make that a bit messier/more divergent from reality if it helps clean up the larger picture, IMO.

I can turn it into a computed concept, but document.open() cannot use this, since the Window object changes, not the Document object. I will fix document.open() separately.

domenic · 2017-05-12T18:52:18Z

source

+  <ol>
+   <li><p>Let <var>window</var> be <var>document</var>'s <span>relevant global object</span>.
+   <!-- Technically this doesn't really work since we create certain Document objects before we
+        create the Window object they are supposed to be created in. -->


Can you add a bug URL to this comment, and update that bug with a checklist? For example it looks like "create a new browsing context" is good, and maybe history traversal is too? Although I guess navigate is bad and history traversal invokes navigation, so that's the reason for this comment?

domenic · 2017-05-12T18:58:26Z

source

@@ -82606,35 +82631,7 @@ State: &lt;OUTPUT NAME=I>1&lt;/OUTPUT> &lt;INPUT VALUE="Increment" TYPE=BUTTON O
     <li><p><span>Implement the sandboxing</span> for the <code>Document</code>.</p></li>

     <li><p><span>Set the allow* flags</span> for the <code>Document</code>.</p></li>
-
-     <li><p>Set <var>settingsObject</var>'s <span


So to make sure I'm understanding why deleting these is OK: navigation always calls "traverse the history" which, in the cases where a new Document was created, will always end up in the "If entry has a different Document object than the current entry" branch, which will then call "set the active document" and take care of all this stuff?

So "process a navigate response" calls the various "Page load ..." sections which first invoke "Initializing a new Document object" (which is defined inline in process navigate response and shares some unfortunate variables; all this needs rewriting) and then later invoke "update the session history with the new page" ("Page load .." sections invoke that).

And that last algorithm always invokes "traverse the history" which ends up being responsible for placing a new document in power.

Note that there's also definitely some event loop issues. That navigate goes asynchronous at some point is reasonable. What happens after that is unclear to me. Obviously we need a task to create the window/document, though this could be a different process/thread (not sure we want to account for that yet though). We need a task for unload. Could maybe be the same task, but not if we want to account for process/thread. And then the switch needs to happen to the new document. Could maybe still be the same task...

jungkees · 2017-05-16T07:24:33Z

source

+   <!-- Same comment as for window applies. -->
+
+   <li><p>Set <var>settingsObject</var>'s <span
+   data-x="concept-environment-execution-ready-flag">execution ready flag</span>.</p></li>


As document.open() creates a new global and an environment settings object, the execution ready flag should be set for it. I think technically, that's a new client. So, if you change document.open() to use these steps, this line of steps would be good as settingsObject should be a new environment settings object created during document.open().

I wonder if @wanderview can say what Firefox does here since only Firefox actually creates a new global object here. I'm really starting to favor the WebKit/Chromium model for this.

annevk · 2017-05-16T11:27:08Z

Commit message:

Centralize setting the active document

This makes browsing context's active document a getter, returning 
browsing context's WindowProxy object's [[Window]] internal slot's 
associated Document.

This removes some service worker text that was added in #1776. 
Reinstating that in some manner is #2687.

Fixes #2657, fixes #2676.

annevk · 2017-05-16T11:35:22Z

source

-     origin</span> as the new <code>Document</code>, then set <var>window</var> to the
-     <code>Window</code> object of that <code>Document</code>, and set <var>settingsObject</var> to
-     <var>window</var>'s <span>relevant settings object</span>.</p></li>
+     origin</span> as the new <code>Document</code>, then do nothing.</p></li>


By the way, if someone can invert this statement so that we only need a single if statement rather than "if then nothing otherwise" that'd be appreciated.

domenic · 2017-05-16T17:52:16Z

source

+   <code>Document</code></span> to <var>document</var>.</p></li>
+
+   <li><p>Set <var>document</var>'s <span>relevant settings object</span>'s <span
+   data-x="concept-environment-execution-ready-flag">execution ready flag</span>.</p></li>


Should this be window's relevant settings object, or document's? In the document.open() case :-/.

Even for document.open(), shouldn't the new window have been associated with the document already at this point?

It should be window I guess, although in the one document.open() implementation where this would matter, it doesn't matter, since Firefox ends up changing the associated global of nodes as well I'm pretty sure. Sigh.

And also, I don't think anything else in the specification really takes it into account that after document.open() the relevant global object might be wrong. We should really stop with document.open() creating a new global...

domenic · 2017-05-16T17:53:08Z

source

@@ -77114,6 +77115,31 @@ dictionary <dfn>DragEventInit</dfn> : <span>MouseEventInit</span> {
  browsing context, since that browsing context was <span data-x="a browsing context is
  discarded">discarded</span>.</p>

+  <p>To <dfn>set the active document</dfn>, given a <span>browsing context</span>
+  <var>browsingContext</var>, <code>Document</code> object <var>document</var>, and optionally a
+  <code>Window</code> object <var>window</var>, run these steps:</p>


Nit: this could be nicer as "set the active document of a browsing context browsingContext to a Document object document, optionally with a Window object window".

This makes browsing context's active document a getter, returning browsing context's WindowProxy object's [[Window]] internal slot's associated Document. This removes some service worker text that was added in #1776. Reinstating that in some manner is #2687. Fixes #2657, fixes #2676.

domenic

LGTM.

I agree the spec is probably all kinds of broken in document.open() situations due to the difference between document's relevant global object and, I guess, document's browsing context's WindowProxy's [[Window]]. We should indeed fix #1698 sooner rather than later.

annevk commented May 12, 2017

View reviewed changes

domenic reviewed May 12, 2017

View reviewed changes

This was referenced May 13, 2017

document.open() doesn't set WindowProxy's [[Window]] #2676

Closed

WindowProxy updated at the wrong point in time #2657

Closed

jungkees reviewed May 16, 2017

View reviewed changes

annevk mentioned this pull request May 16, 2017

Enabling service workers when opener is not a secure context #2687

Open

annevk force-pushed the annevk/set-the-active-document branch from a856919 to 2f695c5 Compare May 16, 2017 09:52

annevk commented May 16, 2017

View reviewed changes

domenic reviewed May 16, 2017

View reviewed changes

annevk force-pushed the annevk/set-the-active-document branch from 1a80da7 to 8684463 Compare May 17, 2017 12:06

feedback

a273051

domenic approved these changes May 17, 2017

View reviewed changes

annevk merged commit 4257a9b into master May 18, 2017

annevk deleted the annevk/set-the-active-document branch May 18, 2017 04:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize setting the active document #2671

Centralize setting the active document #2671

annevk commented May 12, 2017

annevk commented May 12, 2017

annevk commented May 12, 2017

annevk commented May 12, 2017

annevk commented May 12, 2017

annevk May 12, 2017

wanderview May 12, 2017

annevk May 13, 2017

jungkees May 16, 2017

annevk May 16, 2017

domenic May 12, 2017

domenic May 12, 2017

domenic May 12, 2017

annevk May 13, 2017

annevk May 13, 2017

domenic May 15, 2017

annevk May 16, 2017

domenic May 12, 2017

domenic May 12, 2017

annevk May 13, 2017

jungkees May 16, 2017

annevk May 16, 2017

annevk commented May 16, 2017

annevk May 16, 2017

domenic May 16, 2017

jungkees May 17, 2017

annevk May 17, 2017

annevk May 17, 2017

domenic May 16, 2017

domenic left a comment

Centralize setting the active document #2671

Centralize setting the active document #2671

Conversation

annevk commented May 12, 2017

annevk commented May 12, 2017

annevk commented May 12, 2017

annevk commented May 12, 2017

annevk commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment