html/webappapis/scripting/event-loops/fully_active_document.window.js doesn't pass anywhere and has wrong assertions

[noamr]

I don't really understand this test.
Especially https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/scripting/event-loops/fully_active_document.window.js#L11 - why is the iframe not "fully active" here? You've just now waited for it to fully load.

Perhaps this test can be somehow simplified, and in a way that passes in any browser? Otherwise we should remove it.

@jdm can you take a look?

[jdm]

@gterzian You wrote this test—any thoughts on how to proceed?

[gterzian]

I think it makes more sense if you look at the other files involved, see https://github.com/servo/servo/pull/22802/files, and at the example in https://html.spec.whatwg.org/multipage/document-sequences.html#fully-active-documents

• In the test, the frame variable points to an iframe that is inserted in the doc and loaded with "resources/page-with-frame.html" as document.
• "resources/page-with-frame.html" contains itself "iframe.html".
• "iframe.html" listens on the 'storage' event, and when it receives it, it does setItem("Received storage event", true).
• When we navigate frame to "/common/blank.html", "iframe.html" is not fully active anymore, because it's parent is not the active document anymore.
• When the navigation finishes, we set setItem('myCat', 'Tom').
• At that point, it should not deliver a storage event to "iframe.html", because the task queued as part of concept-storage-broadcast should be throttled, because the window event-loop only runs tasks for fully active documents.
• We then navigate back with frame.contentWindow.history.go(-1);
• At that point, the parent of "iframe.html" is the active document again, and so "iframe.html" is fully active, and the storage event should be received(has been, or will be after another spin of the loop). This is checked using getItem("Received storage event"), "true").

Regarding the behavior of Storage, see https://html.spec.whatwg.org/multipage/webstorage.html#concept-storage-broadcast, and especially the note.

[noamr]

@gterzian so how come this doesn't pass in any browser? No browser implements this part of the spec? Perhaps the spec needs to be changed?

[gterzian]

so how come this doesn't pass in any browser?

This I don't know, you'd have to ask the relevant persons. From a quick look in Gecko, I don't see anything that would throttle the task as specified(but I may be missing something).

I speculate it's either because browsers don't queue a task, and instead fire an event then and there, or because they don't throttle the task. But for the Gecko example above, it's hard to tell how it relates exactly to the spec, which is pretty clear(queue a task, which implies it only runs if the document is fully active, and in the task fire an event).

Perhaps the spec needs to be changed?

Maybe, but I would first like the ascertain whether the logic above is correct at all.

Then the question is whether browsers implement this consistently differently, and that this is the intended goal, or whether the spec itself is actually "right", and no one follows it but they should(perhaps because it shows the need to not have different implementations of event-loop and task queues that all differ from the spec in their own unique ways).

I think besides Storage this could be relevant for message port and broadcast channel as well.

[noamr]

I think it makes more sense if you look at the other files involved, see https://github.com/servo/servo/pull/22802/files, and at the example in https://html.spec.whatwg.org/multipage/document-sequences.html#fully-active-documents

• In the test, the frame variable points to an iframe that is inserted in the doc and loaded with "resources/page-with-frame.html" as document.
• "resources/page-with-frame.html" contains itself "iframe.html".
• "iframe.html" listens on the 'storage' event, and when it receives it, it does setItem("Received storage event", true).
• When we navigate frame to "/common/blank.html", "iframe.html" is not fully active anymore, because it's parent is not the active document anymore.

I don't see where "fully active" is specified in this way. See https://html.spec.whatwg.org/multipage/document-sequences.html#fully-active-documents

A Document d is said to be fully active when d is the active document of a navigable navigable, and either navigable is a top-level traversable or navigable's container document is fully active.

Based on this definition I don't see how a fully-active document can have a child iframe with a document that's active but not fully active.

[gterzian]

I don't see how a fully-active document can have a child iframe

I would say it's rather a grand child iframe.

The c-html document in this example, after the b-1.html document has been navigated to b-2.html, is still the active document of its navigable, however it is not fully active anymore because it's container document is not fully active anymore.

In the test, iframe.html is in the same situation as c-html is in the example.

[noamr]

I don't see how a fully-active document can have a child iframe

I would say it's rather a grand child iframe.

The c-html document in this example, after the b-1.html document has been navigated to b-2.html, is still the active document of its navigable, however it is not fully active anymore because it's container document is not fully active anymore.

In the test, iframe.html is in the same situation as c-html is in the example.

This assumes that c-html doesn't get destroyed when its parent navigates away, I don't think you can assume that given https://html.spec.whatwg.org/multipage/document-lifecycle.html#unload-a-document step 20 ?

I debugged the test on chrome and indeed the iframe unloads and the document is destroyed.

[gterzian]

Thanks for looking into it.

So I guess this depends on "Let intendToKeepInBfcache be true if the user agent intends to keep oldDocument alive" at unload-a-document step 5(had only a quick look but it seems other parts that set the salvageable state, like the navigation failing, are not applicable), and intendToKeepInBfcache seems to be up to user agents to decide on as per the last sentence of note-bfcache.

And if destroyed, the task firing the storage event is also removed from the task queue, so the storage event is never received.

So the test makes an assumption about the iframe being cached for history traversal.

But, the unload event only fires if the document is about to be destroyed, so I guess we could add an event-listener for unload at https://github.com/servo/servo/pull/22802/files#diff-27d923d1ab58159e0cc7139c8105de0523aa8a34bd796575e4993442be1dc569R6, and do a sessionStorage.setItem("Received unload event", true); and update the assertion to be "if an unload event was not fired, the storage event should have been received".

[noamr]

Thanks for looking into it.

So I guess this depends on "Let intendToKeepInBfcache be true if the user agent intends to keep oldDocument alive" at unload-a-document step 5(had only a quick look but it seems other parts that set the salvageable state, like the navigation failing, are not applicable), and intendToKeepInBfcache seems to be up to user agents to decide on as per the last sentence of note-bfcache.

And if destroyed, the task firing the storage event is also removed from the task queue, so the storage event is never received.

So the test makes an assumption about the iframe being cached for history traversal.

But, the unload event only fires if the document is about to be destroyed, so I guess we could add an event-listener for unload at https://github.com/servo/servo/pull/22802/files#diff-27d923d1ab58159e0cc7139c8105de0523aa8a34bd796575e4993442be1dc569R6, and do a sessionStorage.setItem("Received unload event", true); and update the assertion to be "if an unload event was not fired, the storage event should have been received".

I think this becomes a test that checks how the event loop behaves with BFCache, so this should follow what other bfache tests such as https://github.com/web-platform-tests/wpt/blob/c4654928a5/html/browsers/history/the-history-interface/history-state-after-bfcache.window.js do with precondition assertions. It might be worthwhile to check if existing BFCache tests already cover what you're trying to test here. Either way, doing this would probably end up with a test that works in at least one browser :)

Do you intend to work on this? Perhaps remove this test and submit a valid test when you're ready?

Note that the unload event is deprecated, so perhaps adding new tests for that is less valuable.

[gterzian]

Yes I think the test could be re-written using something like runBfcacheTest.

But before doing so, it would be good to ascertain whether broadcasting these storage events to documents in the cache(or rather, when they are navigated out of it again) is desirable. There is an open discussion about this for broadcast channels, with browsers showing inconsistent behavior. So I've asked a question at whatwg/html#7253 (comment)

I think it's fine to remove the test for now.