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

Calling InitializeHostDefinedRealm in document.open is probably unnecessary #1698

Closed
rniwa opened this Issue Aug 19, 2016 · 26 comments

Comments

@rniwa
Collaborator

rniwa commented Aug 19, 2016

According to my testing neither WebKit nor Blink does the equivalent of calling InitializeHostDefinedRealm in step 16 of document.open.

WebKit's document.open and its [JavaScript binding code](https://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp?rev= 204629#L153), for example, just sets up the parser state, clears event listeners, & resets some security context, but goes ahead and uses the same global object.

In fact, it's quite possible that a whole bunch of mobile content are dependent on this behavior given the combined market share WebKit and Blink has had in the last decade. In fact, it's easier for author to depend on the fact the global object persists over document.open than that they change. e.g. shared global states, instanceof checks, etc...

@rniwa

This comment has been minimized.

Show comment
Hide comment
@rniwa
Collaborator

rniwa commented Aug 19, 2016

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Aug 21, 2016

Collaborator

WebKit/Blink also don't implement anything like the session history behavior Firefox has long had for document.open; instead they do something much less user-friendly....

It used to be the case that document.open could change origins (though not effective script origins). At that point, reusing the global was an obvious security non-starter from my point of view. I think at this point neither can change on open() (this would need to be verified). If so, we can at least think about this. The session history interactions in the more user-friendly model still end up pretty weird, of course, but I've accepted that WebKit/Blink don't care about the user experience around document.open and that nothing I say will get them to change that....

That said, keeping the same script state but clearing event listeners is pretty weird in its own right: it can cause the still-there scripts to no longer work correctly because the event listeners they depend on are missing.... So there can be web compat fallout from changing to the WebKit/Blink behavior, just as vice versa. :(

Collaborator

bzbarsky commented Aug 21, 2016

WebKit/Blink also don't implement anything like the session history behavior Firefox has long had for document.open; instead they do something much less user-friendly....

It used to be the case that document.open could change origins (though not effective script origins). At that point, reusing the global was an obvious security non-starter from my point of view. I think at this point neither can change on open() (this would need to be verified). If so, we can at least think about this. The session history interactions in the more user-friendly model still end up pretty weird, of course, but I've accepted that WebKit/Blink don't care about the user experience around document.open and that nothing I say will get them to change that....

That said, keeping the same script state but clearing event listeners is pretty weird in its own right: it can cause the still-there scripts to no longer work correctly because the event listeners they depend on are missing.... So there can be web compat fallout from changing to the WebKit/Blink behavior, just as vice versa. :(

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Aug 29, 2016

Member

@bzbarsky could you explain what is not user friendly? Note that per WebKit's binding code it does seem like the origin is changed (they call it "security context").

Member

annevk commented Aug 29, 2016

@bzbarsky could you explain what is not user friendly? Note that per WebKit's binding code it does seem like the origin is changed (they call it "security context").

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Aug 29, 2016

Collaborator

@bzbarsky could you explain what is not user friendly?

Because you can't do back/forward across the document.open call to get to the page before open got called.

Just try this testcase: data:text/html,<script>var x = 0;</script><iframe></iframe><input type="button" value="Click me" onclick="var doc = frames[0].document; doc.open(); doc.write(++x); doc.close()"> and observe the behavior difference between Firefox and Chrome in terms of session history.

Note that per WebKit's binding code it does seem like the origin is changed (they call it "security context").

Changing the origin (as opposed to effective script origin) of a Window object doesn't seem at all OK to me. Nothing in the platform does that right now, to my knowledge.

Collaborator

bzbarsky commented Aug 29, 2016

@bzbarsky could you explain what is not user friendly?

Because you can't do back/forward across the document.open call to get to the page before open got called.

Just try this testcase: data:text/html,<script>var x = 0;</script><iframe></iframe><input type="button" value="Click me" onclick="var doc = frames[0].document; doc.open(); doc.write(++x); doc.close()"> and observe the behavior difference between Firefox and Chrome in terms of session history.

Note that per WebKit's binding code it does seem like the origin is changed (they call it "security context").

Changing the origin (as opposed to effective script origin) of a Window object doesn't seem at all OK to me. Nothing in the platform does that right now, to my knowledge.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Aug 29, 2016

Member

We no longer have an effective script origin concept. I'm not entirely sure what Document* activeDocument = asJSDOMWindow(state.lexicalGlobalObject())->wrapped().document(); does, but given that it seems to require document access, it's likely not that severe.

Member

annevk commented Aug 29, 2016

We no longer have an effective script origin concept. I'm not entirely sure what Document* activeDocument = asJSDOMWindow(state.lexicalGlobalObject())->wrapped().document(); does, but given that it seems to require document access, it's likely not that severe.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Aug 29, 2016

Collaborator

We no longer have an effective script origin concept.

Well, sure. I meant changing origin as opposed to changing "domain".

it's likely not that severe

I'm not sure what you mean by "severe". My point was that currently the origin "pointer" of a document is immutable in the specs, and I see no reason to change that. Having it be immutable allows certain optimizations that would not be possible otherwise, for example.

Collaborator

bzbarsky commented Aug 29, 2016

We no longer have an effective script origin concept.

Well, sure. I meant changing origin as opposed to changing "domain".

it's likely not that severe

I'm not sure what you mean by "severe". My point was that currently the origin "pointer" of a document is immutable in the specs, and I see no reason to change that. Having it be immutable allows certain optimizations that would not be possible otherwise, for example.

@foolip

This comment has been minimized.

Show comment
Hide comment
@foolip

foolip Oct 19, 2016

Member

Looks scary. CC @mikewest and @jeisinger who may understand this code in Blink, or perhaps such a person doesn't exist.

Member

foolip commented Oct 19, 2016

Looks scary. CC @mikewest and @jeisinger who may understand this code in Blink, or perhaps such a person doesn't exist.

@jeisinger

This comment has been minimized.

Show comment
Hide comment
@jeisinger

jeisinger Oct 19, 2016

Member

hum, in #583 we changed document.open() to abort if the origins of the caller and the document don't match, so I don't think we can change the origin here anymore.

Member

jeisinger commented Oct 19, 2016

hum, in #583 we changed document.open() to abort if the origins of the caller and the document don't match, so I don't think we can change the origin here anymore.

@jeisinger

This comment has been minimized.

Show comment
Hide comment
@jeisinger

jeisinger Oct 19, 2016

Member

note that blink's document.open implementation is also pretty far away from what the spec describes :-/

Member

jeisinger commented Oct 19, 2016

note that blink's document.open implementation is also pretty far away from what the spec describes :-/

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 20, 2016

Member

@jeisinger could you describe what you are doing now or maybe point towards the code? Is it different from what WebKit does? I think changing the specification here to not allocate a new global object would be a great simplification to the model we are describing today.

Member

annevk commented Oct 20, 2016

@jeisinger could you describe what you are doing now or maybe point towards the code? Is it different from what WebKit does? I think changing the specification here to not allocate a new global object would be a great simplification to the model we are describing today.

@jeisinger

This comment has been minimized.

Show comment
Hide comment
@jeisinger

jeisinger Oct 20, 2016

Member

our code is here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?rcl=0&l=2499

The main difference appears to be that we have the same-origin check which WK does not.

Member

jeisinger commented Oct 20, 2016

our code is here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?rcl=0&l=2499

The main difference appears to be that we have the same-origin check which WK does not.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 20, 2016

Member

Okay, so why do you still set the origin? Is that even observable?

Member

annevk commented Oct 20, 2016

Okay, so why do you still set the origin? Is that even observable?

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 20, 2016

Collaborator

I think not creating a new global is really weird, because it involves clearing some, but not all, state on the old global. That's really confusing to authors, because some of their things stick around, some do not, and they have no idea which is which.

Collaborator

bzbarsky commented Oct 20, 2016

I think not creating a new global is really weird, because it involves clearing some, but not all, state on the old global. That's really confusing to authors, because some of their things stick around, some do not, and they have no idea which is which.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 20, 2016

Member

Either way what happens is really weird. Best to not use window.open() on yourself (or at all).

Setting the origin to that of the entry settings object is observable as setting document.domain afterwards would affect both sides, rather than one.

I think it's worth making this change so that Document objects are almost associated with one global object. And per https://www.w3.org/Bugs/Public/show_bug.cgi?id=20567#c75 it seems Edge would support this too.

Member

annevk commented Oct 20, 2016

Either way what happens is really weird. Best to not use window.open() on yourself (or at all).

Setting the origin to that of the entry settings object is observable as setting document.domain afterwards would affect both sides, rather than one.

I think it's worth making this change so that Document objects are almost associated with one global object. And per https://www.w3.org/Bugs/Public/show_bug.cgi?id=20567#c75 it seems Edge would support this too.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 20, 2016

Collaborator

Setting the origin to that of the entry settings object is observable

Yes, we don't want to do that at all.

I think it's worth making this change

I don't think it is and would need some very careful analysis of any new proposed behavior, its interactions with navigation, and its possible web compat impacts to be comfortable making any changes here.

Collaborator

bzbarsky commented Oct 20, 2016

Setting the origin to that of the entry settings object is observable

Yes, we don't want to do that at all.

I think it's worth making this change

I don't think it is and would need some very careful analysis of any new proposed behavior, its interactions with navigation, and its possible web compat impacts to be comfortable making any changes here.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 20, 2016

Collaborator

I should note that you never did address my concerns about user experience and back/forward navigation....

Collaborator

bzbarsky commented Oct 20, 2016

I should note that you never did address my concerns about user experience and back/forward navigation....

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 20, 2016

Member

Basically, it does not seem worth caring about document.open() doing the "right" thing. And if we can drastically simplify the overall model by adopting what the majority of browsers do, we should.

Member

annevk commented Oct 20, 2016

Basically, it does not seem worth caring about document.open() doing the "right" thing. And if we can drastically simplify the overall model by adopting what the majority of browsers do, we should.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 20, 2016

Collaborator

Basically, it does not seem worth caring about document.open() doing the "right" thing.

I think we simply disagree on that.

And if we can drastically simplify the overall model by adopting what the majority of browsers do, we should.

If it's actually a simplification and if the "majority of browsers" are actually all doing the same thing and if the spec properly figures out that thing and if that thing doesn't depend on other non-spec-matching behaviors in those browsers. I've been burned so many times recently by HTML spec changes that failed to actually check some of those things, that I'm very wary of them, sorry. :(

Collaborator

bzbarsky commented Oct 20, 2016

Basically, it does not seem worth caring about document.open() doing the "right" thing.

I think we simply disagree on that.

And if we can drastically simplify the overall model by adopting what the majority of browsers do, we should.

If it's actually a simplification and if the "majority of browsers" are actually all doing the same thing and if the spec properly figures out that thing and if that thing doesn't depend on other non-spec-matching behaviors in those browsers. I've been burned so many times recently by HTML spec changes that failed to actually check some of those things, that I'm very wary of them, sorry. :(

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 20, 2016

Member

That's fair and I certainly wouldn't expect Gecko to make a move here anytime soon. We basically need to do a lot more to remove technical debt around the lifecycle of documents, global objects, and browsing contexts. And make sure all of that also ends up getting tested properly.

In order to get there though we will need to make some decisions along the way, this being one of them.

Member

annevk commented Oct 20, 2016

That's fair and I certainly wouldn't expect Gecko to make a move here anytime soon. We basically need to do a lot more to remove technical debt around the lifecycle of documents, global objects, and browsing contexts. And make sure all of that also ends up getting tested properly.

In order to get there though we will need to make some decisions along the way, this being one of them.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Oct 20, 2016

Collaborator

OK, but I have yet to see a clear description of what the "other model" even is. That seems like a minimal bar to even having a meaningful conversation here....

Collaborator

bzbarsky commented Oct 20, 2016

OK, but I have yet to see a clear description of what the "other model" even is. That seems like a minimal bar to even having a meaningful conversation here....

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Jan 24, 2017

Member

@rniwa Does WebKit even prompt to unload, unload and then abort the Document before creating the new parser etc? Does it cancel ongoing loads? Asking because I'm implementing this in Servo.

Member

nox commented Jan 24, 2017

@rniwa Does WebKit even prompt to unload, unload and then abort the Document before creating the new parser etc? Does it cancel ongoing loads? Asking because I'm implementing this in Servo.

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Jan 31, 2017

Member

AFAICT, WebKit also doesn't do anything with regard to timers, and it just let them run until their completion.

Member

nox commented Jan 31, 2017

AFAICT, WebKit also doesn't do anything with regard to timers, and it just let them run until their completion.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic May 2, 2017

Member

So this is still unresolved, but for the purposes of writing tests, my thoughts are that "the other model" should include resetting everything that is Window-associated. For example, the custom elements registry. So after document.open(), whether you follow the Firefox/spec model or the currently-underspecified everyone-else model, window.customElements should change value.

We're adding tests for that in web-platform-tests/wpt#5681, with bugs to be filed shortly.

Member

domenic commented May 2, 2017

So this is still unresolved, but for the purposes of writing tests, my thoughts are that "the other model" should include resetting everything that is Window-associated. For example, the custom elements registry. So after document.open(), whether you follow the Firefox/spec model or the currently-underspecified everyone-else model, window.customElements should change value.

We're adding tests for that in web-platform-tests/wpt#5681, with bugs to be filed shortly.

@jeisinger

This comment has been minimized.

Show comment
Hide comment
@jeisinger

jeisinger May 16, 2017

Member

@annevk asked me to spell out the document.open steps of Blink. Here we go:

  1. if the document was imported (via HTML imports), abort with an InvalidStateError
  2. like step 1) in the spec
  3. like step 2) in the spec
  4. like step 4) in the spec
  5. set document's origin to the entered settings object's responsible document (or whatever the right spec term is) origin
  6. if the document and the entered settings object's responsible document differ, set the document's URL to the entered... document'sURL with the fragment stripped. (almost step 23 of the spec)
  7. set the document's cookie URL to the entered... document's cookie URL
  8. if the document is active, run step 5 of the spec
  9. if the document is active, and has an active parser that wasn't created by script, and has an insertion point, return document
  10. "stop all loaders". I guess that's step 12) in the spec?
  11. step 13 of the spec
  12. clear any selection associated with document
  13. step 15 of the spec
  14. set document's compat mode to "no quirks"
  15. step 25 of the spec
  16. step 26 of the spec
  17. if "load event progress" is not "load event in progress" and we're not dispatching a page dismissal event, set "load event progress" to "load event not run" (not sure what this is in spec terms)
  18. mark the loader associated with document to have committed its first real load if it hasn't done so yet
  19. notify our loading machinery that a load has started unless the document's parent is done loading or in the process of completing
  20. cancel ongoing navigations for this document
  21. return document

hope that helps :/

Member

jeisinger commented May 16, 2017

@annevk asked me to spell out the document.open steps of Blink. Here we go:

  1. if the document was imported (via HTML imports), abort with an InvalidStateError
  2. like step 1) in the spec
  3. like step 2) in the spec
  4. like step 4) in the spec
  5. set document's origin to the entered settings object's responsible document (or whatever the right spec term is) origin
  6. if the document and the entered settings object's responsible document differ, set the document's URL to the entered... document'sURL with the fragment stripped. (almost step 23 of the spec)
  7. set the document's cookie URL to the entered... document's cookie URL
  8. if the document is active, run step 5 of the spec
  9. if the document is active, and has an active parser that wasn't created by script, and has an insertion point, return document
  10. "stop all loaders". I guess that's step 12) in the spec?
  11. step 13 of the spec
  12. clear any selection associated with document
  13. step 15 of the spec
  14. set document's compat mode to "no quirks"
  15. step 25 of the spec
  16. step 26 of the spec
  17. if "load event progress" is not "load event in progress" and we're not dispatching a page dismissal event, set "load event progress" to "load event not run" (not sure what this is in spec terms)
  18. mark the loader associated with document to have committed its first real load if it hasn't done so yet
  19. notify our loading machinery that a load has started unless the document's parent is done loading or in the process of completing
  20. cancel ongoing navigations for this document
  21. return document

hope that helps :/

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Dec 16, 2017

Member

FWIW, I was curious how the different document.open() behavior might impact service workers and clients API. I did some testing:

The test I ran was:

  1. Open https://fetch-event-echo.glitch.me/
  2. Open web console
  3. Check value of navigator.serviceWorker.controller
  4. Execute fetch('/') and note the client ID echo'd by the service worker.
  5. Execute document.open();document.close();
  6. Repeat steps 3 and 4 and compare.

I mainly checked firefox and chrome since that is what I have available on this machine:

Firefox 57:

  • controlled by same service worker before and after
  • same client ID before and after

Firefox 59:

  • controlled by same service worker before and after
  • different client ID after

Chrome 63 and 65:

  • controlled by same service worker before and after
  • same client ID before and after

The fact we inherit the controller is better interop than I expected.

The client ID should reflect if a new global is created or not. Using the same client ID in chrome makes sense since it does not create a new global. In FF57 we had a bug because we were treating the document as the Client (and document.open does not create a new document object). In FF59 we now treat the window global as the client, so we get a new ID after document.open().

Member

wanderview commented Dec 16, 2017

FWIW, I was curious how the different document.open() behavior might impact service workers and clients API. I did some testing:

The test I ran was:

  1. Open https://fetch-event-echo.glitch.me/
  2. Open web console
  3. Check value of navigator.serviceWorker.controller
  4. Execute fetch('/') and note the client ID echo'd by the service worker.
  5. Execute document.open();document.close();
  6. Repeat steps 3 and 4 and compare.

I mainly checked firefox and chrome since that is what I have available on this machine:

Firefox 57:

  • controlled by same service worker before and after
  • same client ID before and after

Firefox 59:

  • controlled by same service worker before and after
  • different client ID after

Chrome 63 and 65:

  • controlled by same service worker before and after
  • same client ID before and after

The fact we inherit the controller is better interop than I expected.

The client ID should reflect if a new global is created or not. Using the same client ID in chrome makes sense since it does not create a new global. In FF57 we had a bug because we were treating the document as the Client (and document.open does not create a new document object). In FF59 we now treat the window global as the client, so we get a new ID after document.open().

annevk added a commit that referenced this issue Apr 27, 2018

Investigate a new document.open()
For #1698, #3564, ... Needs many tests.

annevk added a commit that referenced this issue May 2, 2018

Investigate a new document.open()
For #1698, #3564, ... Needs many tests.

annevk added a commit that referenced this issue May 3, 2018

Investigate a new document.open()
For #1698, #3564, ... Needs many tests.

annevk added a commit that referenced this issue May 3, 2018

Investigate a new document.open()
For #1698, #3564, ... Needs many tests.
@futurist

This comment has been minimized.

Show comment
Hide comment
@futurist

futurist May 15, 2018

For the session history problem, I think it should be managed by the developer when he/she wrote document.open, then history can be managed by history API like history.pushState, history.replaceState etc.

Since the box is opened by developer, it's their responsibility to manage that, thus more control of the history rather than managed by the browser.

futurist commented May 15, 2018

For the session history problem, I think it should be managed by the developer when he/she wrote document.open, then history can be managed by history API like history.pushState, history.replaceState etc.

Since the box is opened by developer, it's their responsibility to manage that, thus more control of the history rather than managed by the browser.

@TimothyGu TimothyGu referenced this issue Jul 12, 2018

Closed

Tracking issue for document.open() overhaul #3818

19 of 19 tasks complete

TimothyGu added a commit to TimothyGu/html that referenced this issue Aug 14, 2018

Remove document.open() realm creation
Based on the work by Anne van Kesteren in #3651, but without the parts
concerning the session history. Changes to that area will come later
(see #3818).

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10789
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes #1698.
Fixes #3286.
Fixes #3306.
Closes #3665.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

TimothyGu added a commit to TimothyGu/html that referenced this issue Aug 14, 2018

Remove document.open() realm creation
Based on the work by Anne van Kesteren in #3651, but without the parts
concerning the session history. Changes to that area will come later
(see #3818).

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes #1698.
Fixes #3286.
Fixes #3306.
Closes #3665.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>

@domenic domenic closed this in #3918 Aug 16, 2018

domenic added a commit that referenced this issue Aug 16, 2018

document.open() simplifications, part 1
In particular, removes the realm creation, document unloading, and tasks
removal steps.

Based on the work by Anne van Kesteren in #3651, but without the parts
concerning the session history. Changes to that area will come later
(see #3818).

These changes allow us to remove several auxiliary concepts that only
existed to support document.open():

- The recycle parameter to the "unload a Document" algorithm
- The window parameter to the "set the active document" algorithm

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes #1698.
Fixes #3286.
Fixes #3306.
Closes #3665.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment