serenity icon indicating copy to clipboard operation
serenity copied to clipboard

LibWeb: Document::create_and_initialize() uses no-longer-valid browsing context API

Open awesomekling opened this issue 2 years ago • 3 comments

This section here:

    // 7. If browsingContext's active document's is initial about:blank is true,
    //    and browsingContext's active document's origin is same origin-domain with navigationParams's origin,
    //    then set window to browsingContext's active window.
    if (browsing_context->still_on_its_initial_about_blank_document()
        && (browsing_context->active_document() && browsing_context->active_document()->origin().is_same_origin(navigation_params.origin))) {
        window = browsing_context->active_window();
    }

Instead of browsing_context->still_on_its_initial_about_blank_document(), we should be checking browsing_context->active_document()->is_initial_about_blank().

Also, instead of is_same_origin on the next line, we should be using is_same_origin_domain.

These two fixes would bring us in line with the spec (you can even see that the code doesn't match the spec comment right now.)

However, if I make these tweaks, the Speedometer benchmark no longer starts at all. So some further investigation is needed to figure out what's wrong.

awesomekling avatar Dec 15 '23 08:12 awesomekling

After the conversion to navigables and subsequent cleanup, still_on_its_initial_about_blank_document() is essentially a return false; function. We don't update the BrowsingContext's session history, and haven't for a long time.

The spec text for this used to be:

A browsing context browsingContext is still on its initial about:blank Document if browsingContext's session history's size is 1 and browsingContext's session history[0]'s document's is initial about:blank is true.

Ran into this trying to remove unused BrowsingContext APIs.

It seems that re-using the browsing context's active window causes the browsing context's active window's associated document to not be 'fully loaded' when we try to create some queued global tasks for it in apply_the_history_step.

This is starting to smell like a spec issue. But, there's still more browsing context cleanup to do before I'd want to open an issue on HTML.

cc @kalenikaliaksandr

ADKaster avatar Feb 04 '24 10:02 ADKaster

I also saw in the spec:

https://html.spec.whatwg.org/#concept-document-window

The Window object has an associated Document, which is a Document object. It is set when the Window object is created, and only ever changed during navigation from the initial about:blank Document.

But, looking around the spec and in our own implementation, I couldn't find anywhere during navigation from the initial about:blank that ever changes the associated document of a window.

ADKaster avatar Feb 04 '24 10:02 ADKaster

... actually I think resetting the associated document is supposed to happen in

  • populate_session_history_entry_document
    • load_document
      • load_html_document
        • Document::create_and_initialize

But we never get to load_document with the change suggested in the initial issue post, because the associated document of the navigable's active window is ... changed too early? That doesn't make any sense, but matches what I saw with my debug logs.

Definitely leaving this one for later

ADKaster avatar Feb 04 '24 10:02 ADKaster