Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WindowProxy updated at the wrong point in time #2657

Closed
annevk opened this issue May 10, 2017 · 7 comments
Closed

WindowProxy updated at the wrong point in time #2657

annevk opened this issue May 10, 2017 · 7 comments

Comments

@annevk
Copy link
Member

annevk commented May 10, 2017

The overall set of steps for getting something to display in a browsing context (other than about:blank) is simplified:

  1. Dispatch based on MIME type.
  2. In a task:
    1. Create a document.
    2. Initialize the document and create a corresponding Window object.
    3. Update the WindowProxy slot.
  3. In another task:
    1. Update the session history with the new page.

2.3 above seems wrong as that means the "unload a document" algorithm (run in 3.1) would run against the wrong global.

(It's also bogus where these tasks would actually go if the event loop is per agent cluster and we have a cross-origin navigation.)


What I think we should have by the way is more something like this (though don't let this alternative plan stop you from fixing the problem above):

  1. Figure out whether to create a new global object or not and act on that.
  2. Create a document in that global object.
  3. Initialize the document as per MIME type and creator/parent.
  4. Unload the old document and replace with the new.

1 initially seemed problematic as we don't really know whether we want a new global, but as long as we haven't switched WindowProxy and started running scripts it seems okay. And it definitely seems better to create a global first as otherwise we have to define the lazy allocation of JavaScript objects which seems silly and counter to the goal of harmonizing with JavaScript.

@annevk
Copy link
Member Author

annevk commented May 10, 2017

So another problem I just realized is that we update what the global object itself points to in the initial about:blank scenario:

  1. Set window's associated Document to the new Document.

That should be moved as well to happen after unload runs to completion (whatever that means).

@annevk
Copy link
Member Author

annevk commented May 12, 2017

Sigh.

I think what needs to happen is that these move to "traverse the history" step 4 substep 3, which currently reads:

Make entry's Document object the active document of the browsing context.

That is the point at which the document becomes the active document. Arguably we should refactor that step to call something that does:

  1. Let window be document's relevant global object. (Note: 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. However, further refactoring should get rid of that nonsense.)
  2. Set browsing contextx's WindowProxy's internal [[Window]] slot to window.
  3. Set Window object's associated Document to document.
    1 Set browsing context's active document to document.

We could then also use that in "create a new browsing context" which currently doesn't seem to set the active document (despite the definition of active document saying it's always designated somehow).

There is a further problem, likely introduced by @jungkees though by no means his fault, at the end of "Initializing a new Document object":

Set settingsObject's execution ready flag.
...

These should probably also move to the above algorithm that changes the active document. At least, I think we don't want to set the execution ready flag until the event loop actually spins.

This should bring us a little closer.

@jungkees
Copy link
Contributor

These should probably also move to the above algorithm that changes the active document. At least, I think we don't want to set the execution ready flag until the event loop actually spins.

My intention was set it just before when a script is ready to run. Also, I thought it should be later than the global object is created, the document is created and (A) initialized, that global is set to WindowProxy.

(A) Particularly in the Initializing a new Document object algorithm, I thought setting the execution ready flag should be later than "Implement the sandboxing" and "Set the allow* flags" called in the step 11 and 12.

FYI, the purpose of the execution ready flag is to expose the state of a client through the Client API: whether the client is in the reserved state or not. (See https://w3c.github.io/ServiceWorker/#ref-for-client-reserved-state-2 and https://w3c.github.io/ServiceWorker/#ref-for-client-reserved-state-1)

@annevk
Copy link
Member Author

annevk commented May 16, 2017

@jungkees yeah, that happens at a later stage, see #2671 for changes.

There's two things not clear:

  1. The other steps you added that are not implemented in browsers apparently. Disowning the opener if there's an active service worker or some such...
  2. Whether the execution ready flag should be set when document.open() is invoked.

@jungkees
Copy link
Contributor

The other steps you added that are not implemented in browsers apparently. Disowning the opener if there's an active service worker or some such...

It was a follow-up on the f2f Toronto 2016: w3c/ServiceWorker#890 (comment). If that part doesn't make sense, then we'll need to discuss where to put the equivalent steps.

I'll have a time to look at the PR and document.open() today. I'll comment on the PR for them.

@annevk
Copy link
Member Author

annevk commented May 16, 2017

If that part doesn't make sense, then we'll need to discuss where to put the equivalent steps.

Okay, unless you object I'm going to remove those steps and file a follow-up issue so we can sort out what you want and how to (maybe) get there.

@jungkees
Copy link
Contributor

remove those steps and file a follow-up issue

That works for me. Please leave the history it was our effort on the f2f follow-up. Happy to help this navigation algorithm refactoring. Eventually, I hope we can sort out the service worker inheritance for iframe case in #2080 based on this refactoring.

annevk added a commit that referenced this issue May 16, 2017
annevk added a commit that referenced this issue May 17, 2017
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 added a commit that referenced this issue May 18, 2017
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.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
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 whatwg#1776.
Reinstating that in some manner is whatwg#2687.

Fixes whatwg#2657, fixes whatwg#2676.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants