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

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

Open
awesomekling opened this issue Dec 15, 2023 · 3 comments

Comments

@awesomekling
Copy link
Contributor

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.

@ADKaster
Copy link
Member

ADKaster commented Feb 4, 2024

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
Copy link
Member

ADKaster commented Feb 4, 2024

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
Copy link
Member

ADKaster commented Feb 4, 2024

... 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Development

No branches or pull requests

2 participants