1965437 - sync-about-blank: navigation_started.py test_window_open_with_url timeout. NavigationListenerChild misses the start state change

Status: NEW

(Remote Protocol :: WebDriver BiDi, defect)

Description

[Vincent Hilla [:vhilla]]

In Bug 543435, we are trying to change how the initial about:blank is loaded. These changes caused test_window_open_with_url to fail. The failure is permanent, only with non-fission, and I can reproduce it locally on Linux. But on try without fission, it only shows up on Android, not Linux.

Here is

• a try push for linux, android
• pernosco session of the test on mozilla central
• pernosco session of the test with the changes applied

See bug 1261180 c8 for how I recorded a webdriver test under rr.

I'll also attach a few logs that I created locally with print debugging and deem useful.

---

I'll briefly summarize what I think is going on.

On central, we'll dispatch two DOMWindowCreated events. One from ProvideWindow, and one from SetInitialPrincipal, both in OpenWindowInternal. In ProvideWindowCommon called from ProvideWindow, we SendCreateBrowser and wait for a response, which leads us to obtain a MessageManagerGroup before the second DOMWindowCreated is sent. Without this group, the JSWindowActor wouldn't match the browsing context and JSWindowActorProtocol::HandleEvent would bail.

With the patch, we'll only dispatch the first DOMWindowCreated, where we don't yet have a MessageManagerGroup. So our MessageHandlerFrameChild doesn't get a DOMWindowCreated for the initial transient about:blank. We'll start loading the target URI, do BCWebProgress STATE_START and only when we reach HttpChannelChild::OnStartRequest do we dispatch another DOMWindowCreated, this time for the actual URI. The MessageHandlerFrameChild responds, WindowGlobalMessageHandler sends browsingContext._applySessionData, a MessageHandler is created and the NavigationListenerChild starts listening. But STATE_START is already done, so the test times out.

Our patch removes the call to SetInitialPrincipal and ensures the document created earlier already has the right principal via openWindowInfo passed to ProvideWindow. Though I don't know why this was needed.

With fission, we have two BCWebProgress STATE_START notifications. I discarded those logs, but remember that the docloader changes once we get network data for inline.py. In both cases, we'll reach nsBrowserStatusFilter::OnStateChange. But only with fission, the new docloader will have mIsLoadingDocument be false and we call doStartDocumentLoad. Without fission, we'll call doStartURLLoad instead.

---

Maybe

• WebDriver should already catch the first DOMWindowLoaded?
• Or the browsing context should already have MessageManagerGroup when dispatching the first DOMWindowLoaded?
• Or the changes around SetInitialPrincipal in OpenWindowInternal need adjustment?

Comment 1

[Vincent Hilla [:vhilla]]

Attachment: navigation-started-fis-patch.log

Comment 2

[Vincent Hilla [:vhilla]]

Attachment: navigation-started-nofis-patch.log

Comment 3

[Vincent Hilla [:vhilla]]

Attachment: navigation-started-fis-central.log

Comment 4

[Vincent Hilla [:vhilla]]

Attachment: navigation-started-nofis-central.log

Comment 5

[Vincent Hilla [:vhilla]]

:nika, you reviewed Bug 1580448. Do you have thoughts on DOMWindowCreated being dispatched before the browsing context has a MessageManagerGroup and thus JSWindowActorProtocol::MessageManagerGroupMatches being false?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

We can probably change how we set MessageManagerGroup such that it is available during the true initial about:blank document's creation. Currently these properties are only set when the embedder element is defined in https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/docshell/base/BrowsingContext.cpp#774-776, however, it may be possible to do this a bit earlier in the lifecycle of a new BrowsingContext.

Currently SetEmbedderElement can only be run on an already-"attached" BrowsingContext, but it might be possible to allow it to be run before the BrowsingContext is attached by replacing https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/docshell/base/BrowsingContext.cpp#793 with:

if (EverAttached()) {
  MOZ_ALWAYS_SUCCEEDS(txn.Commit(this));
} else {
  txn.CommitWithoutSyncing(this);
}

As well as moving https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/docshell/base/BrowsingContext.cpp#808-810 to be in the if (aEmbedder) block and use the txn so that it doesn't start a separate transaction, like:

if (HTMLObjectElement::FromNode(aEmbedder) ||
    HTMLEmbedElement::FromNode(aEmbedder)) {
  txn.SetIsSyntheticDocumentContainer(true);
}

Unfortunately I expect this will still be a bit broken, as we'll need to suppress the browsing-context-did-set-embedder notification (https://searchfox.org/mozilla-central/source/docshell/base/BrowsingContext.cpp#803-806) if !EverAttached(), as it's unsafe to expose a non-attached BrowsingContext to JS. We can get around that by optionally firing an extra check here: https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/docshell/base/BrowsingContext.cpp#985-988, like:

  if (nsCOMPtr<nsIObserverService> obs = services::GetObserverService()) {
    obs->NotifyWhenScriptSafe(ToSupports(this), "browsing-context-attached",
                              why);

    // If we've already had an embedder element set before we were attached, tell listeners that there
    // is an embedder element on this BrowsingContext now.
    if (mEmbedderElement) {
      obs->NotifyWhenScriptSafe(ToSupports(this), "browsing-context-did-set-embedder",
                                nullptr);
    }
  }

Finally, we might also want this to be inherited into pop-up windows, which we could do by adding a line here: https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/docshell/base/BrowsingContext.cpp#376

fields.Get<IDX_MessageManagerGroup>() = aOpener->Top()->GetMessageManagerGroup();

TBH there is also a chance we'd need to change some things in https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/docshell/base/CanonicalBrowsingContext.cpp#294-296 to ensure that the values set in SetEmbedderElement carry over when the BC is replaced, but I'm not 100% sure yet.

---

EDIT: I just realized after posting that I completely forgot to mention the goal of the change to SetEmbedderElement. We'd need to move the calls to set the embedder element here (https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/dom/base/nsFrameLoader.cpp#2198-2202) to before the EnsureBrowsingContextAttached(), and here (https://searchfox.org/mozilla-central/rev/4bd620c218a01568d541453ce3b215ebf55bca63/dom/base/nsFrameLoader.cpp#2572-2574), adding probably a new call before the EnsureBrowsingContextAttached().