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

No longer remove tasks for document.open() #3665

Closed
wants to merge 1 commit into from

Conversation

5 participants
@annevk
Copy link
Member

commented May 3, 2018

This is not consistently implemented and given that Firefox does not appear to do it at all I hope we can get away without it.

Tests: web-platform-tests/wpt#10818.


/dynamic-markup-insertion.html ( diff )

No longer remove tasks for document.open()
This is not consistently implemented and given that Firefox does not appear to do it at all I hope we can get away without it.

Tests: web-platform-tests/wpt#10818.
@annevk

This comment has been minimized.

Copy link
Member Author

commented May 3, 2018

@foolip @cdumez Chrome and Safari do seem to remove messages from postMessage(). Any idea about that? @dstorey Edge seems to implement this behavior, would you all mind if it was removed?

@foolip

This comment has been minimized.

Copy link
Member

commented May 9, 2018

I haven't heard about or considered that behavior before, so couldn't say if it's intentional or accidental. But if it's tested it shouldn't regress :)

@annevk

This comment has been minimized.

Copy link
Member Author

commented May 9, 2018

@foolip regress?

@foolip

This comment has been minimized.

Copy link
Member

commented May 9, 2018

I mean that if we spec the "remove message" behavior and it's just accidental in Chrome, then as long as there's a test for the behavior it won't regress to some non-spec behavior.

@TimothyGu

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

The actual current task-removal behavior is actually pretty subtle, as many queued tasks are for firing events, and document.open() also has a step that removes event listeners.

Here's a collection of tests I've developed for this issue (those starting with "step-14") and their results on each browser.

Version info

Chrome 69.0.3488.0 (Official Build) canary (64-bit)

Safari Tech Preview Release 60 (Safari 12.0, WebKit 13606.1.23.1)

Firefox Nightly 63.0a1 (2018-07-11) (64-bit)

Microsoft Edge 42.17134.1.0
Microsoft EdgeHTML 17.17134

= task still observable
= task cancelled or unobservable
N/A = specific feature is unimplemented

(event) = observation is done through event listeners added to non-nodes before document.open()
(late x event) = observation is done through event listeners added to x after document.open()

  Chrome Safari Firefox Edge Spec Proposed
canvas toBlob N/A
sessionStorage (event)  
MessageChannel (event)
interval
timeout
window.postMessage (late Window event) (part of proposed WPT)
Promise rejection (late Window event) N/A N/A
Marquee start event (late element event) N/A N/A

Then there are a few tests with questionable value to this specific issue as they may be affected by other implementation differences.

(Window event) = observation is done through event listeners added to impacted Window before document.open()
(fetch) = test that might be affected by fetch abortion (by way of "abort document")

  Chrome Safari Firefox Edge Spec Proposed
Image loading (fetch, late element event) (SecurityError)
WebSocket (fetch, event) (close event observable)
Promise rejection (Window event) N/A N/A
window.postMessage (Window event)

The last row's proposal is in conjunction with the resolution from #3818 to remove event listeners on the Window object.

@TimothyGu

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Blink and WebKit do not remove any tasks in document.open(), but they visibly remove event listeners on Window, causing the ✘ on the last two tests.

Gecko does not remove most tasks but removes some tasks (timers and marquee's start event). It also seems to cancel the fetches made for loading image for img element, and for WebSocket (see web-platform-tests/wpt#10789).

Edge isn't observed to allow any task to remain execution except for window.postMessage(), which is the current spec behavior. But this might be because of the cross-frame testing scheme I used rather than that they actually remove tasks. The SecurityError shown in the console when the image loading test is done is an especially strong hint for it.

In conclusion, I propose we adopt the current behavior of Chrome and Safari, which is twofold:

  1. Do not remove any tasks. This should make implementation of document.open() simpler in general, and allow an easier path to consistency for Firefox and Edge. (This PR.)
  2. Remove event listeners on Window when removing event listeners from nodes. Chrome and Safari already visibly, explicitly do this. It's unclear if Firefox or Edge does so too, but as one can see from the tests tagged (Window event) if we remove them it would be an unobservable change.

The WPT PR (web-platform-tests/wpt#10818) has been appropriately updated to reflect these proposed changes.

@wanderview

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Wouldn't this change depend on also changing the spec to keep the original global and not create a new global? It seems odd to say tasks should be run for a global that has been removed.

@TimothyGu

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

@wanderview

Wouldn't this change depend on also changing the spec to keep the original global and not create a new global? It seems odd to say tasks should be run for a global that has been removed.

Yeah, I agree that it is a very odd behavior. However, that's more or less what Firefox is doing right now for some tasks, and I agree with @annevk that the easiest way to interoperability is to remove both of these clauses.

I understand that it can be difficult to look at each proposed change to document.open() in isolation. If it makes it easier, I've got a rough plan for general document.open() renovation written up in #3818 which contains both of these changes. I didn't request for a review from implementors on that issue yet, as my investigation is not yet complete for all issues at hand and I'd like to present a comprehensive plan. However, do feel free to comment there on the issues I've already written up a "resolution" for!

@wanderview

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

Yeah, I tried to make a lot of tasks abort for document.open() when I was cleaning up FF's global teardown code, but I was told we couldn't due to web compat issues. It's super weird.

I guess I was just trying to say fixing this issue needs to be done in coordination with changing the global behavior to make sense in the spec.

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

Remove document.open() realm creation
Based on the work by Anne van Kesteren in whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#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 whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.

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

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

Remove document.open() realm creation
Based on the work by Anne van Kesteren in whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#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 whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.

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

domenic added a commit that referenced this pull request 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.

@TimothyGu TimothyGu deleted the annevk/document-open-steps-tasks branch Aug 17, 2018

mustaqahmed added a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019

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 whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#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 whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.

mustaqahmed added a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019

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 whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#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 whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.