1921756 - isBrowsingContextCompatible can throw if currentWindowGlobal is not available and fail broadcasts

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P2)

Description

[Julian Descottes [:jdescottes]]

Reported by peers from the Puppeteer team, they see increased failures with the following stack trace:

2024-09-30T08:15:11.7910327Z ProtocolError: Protocol error (session.subscribe): unknown error TypeError: can't access property "documentPrincipal", browsingContext.currentWindowGlobal is null isExtensionContext@chrome://remote/content/shared/messagehandler/transports/BrowsingContextUtils.sys.mjs:8:5
2024-09-30T08:15:11.7913927Z isBrowsingContextCompatible@chrome://remote/content/shared/messagehandler/transports/BrowsingContextUtils.sys.mjs:78:6
2024-09-30T08:15:11.7916439Z _getBrowsingContexts@chrome://remote/content/shared/messagehandler/transports/RootTransport.sys.mjs:192:16
2024-09-30T08:15:11.7918653Z _getBrowsingContextsForDescriptor@chrome://remote/content/shared/messagehandler/transports/RootTransport.sys.mjs:162:19
2024-09-30T08:15:11.7920750Z _broadcastCommand@chrome://remote/content/shared/messagehandler/transports/RootTransport.sys.mjs:87:12
2024-09-30T08:15:11.7922618Z forwardCommand@chrome://remote/content/shared/messagehandler/transports/RootTransport.sys.mjs:76:19
2024-09-30T08:15:11.7924435Z forwardCommand@chrome://remote/content/shared/messagehandler/RootMessageHandler.sys.mjs:162:36
2024-09-30T08:15:11.7926139Z handleCommand@chrome://remote/content/shared/messagehandler/MessageHandler.sys.mjs:260:17
2024-09-30T08:15:11.7927940Z updateSessionData@chrome://remote/content/shared/messagehandler/sessiondata/SessionData.sys.mjs:323:20
2024-09-30T08:15:11.7929849Z updateSessionData@chrome://remote/content/shared/messagehandler/RootMessageHandler.sys.mjs:206:29
2024-09-30T08:15:11.7931839Z update@chrome://remote/content/shared/messagehandler/EventsDispatcher.sys.mjs:207:32
2024-09-30T08:15:11.7933613Z subscribe@chrome://remote/content/webdriver-bidi/modules/root/session.sys.mjs:95:48
2024-09-30T08:15:11.7935227Z handleCommand@chrome://remote/content/shared/messagehandler/MessageHandler.sys.mjs:257:33
2024-09-30T08:15:11.7936677Z execute@chrome://remote/content/shared/webdriver/Session.sys.mjs:390:32
2024-09-30T08:15:11.7938245Z onPacket@chrome://remote/content/webdriver-bidi/WebDriverBiDiConnection.sys.mjs:228:37
2024-09-30T08:15:11.7939631Z onMessage@chrome://remote/content/server/WebSocketTransport.sys.mjs:127:18
2024-09-30T08:15:11.7940907Z handleEvent@chrome://remote/content/server/WebSocketTransport.sys.mjs:109:14

While it's not clear what caused the flakiness, for a broadcast we should never fail when retrieving the list of compatible BrowsingContexts. In this case it seems one of the contexts does not have a currentWindowGlobal property so we should just ignore it.

Comment 1

[Julian Descottes [:jdescottes]]

Apparently this is windows only

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I assume this most likely got regressed by bug 1854942.

I wonder if we should actually flip this preference remote.retry-on-abort to false for the current beta cycle to give it a bit more time to settle on Nightly. I probably should not have pushed the patch on bug 1854942 end of last week.

Comment 3

[Julian Descottes [:jdescottes]]

So far I can't reproduce the issue either in wdspec or regular browser mochitests.
I've been trying to run many broadcasts in parallel with tab creation/reload/closing. Probably missing something.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1921756 - [bidi] Avoid errors on uninitialized browsingContexts during command broadcast

The isExtensionContext and isParentProcess browsing context helpers could throw if the
browsingContext did not have a valid currentWindowGlobal property.

Here we perform an early check on currentWindowGlobal from the only caller of those helpers, which
is isBrowsingContextCompatible. For this helper, it makes sense to return false if the browsing
context is not correctly attached yet. But it is more difficult to return a meaningful value
from isExtensionContext or isParentProcess, because those 2 helpers cannot assess the browsing
context correctly.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa72f80a6336
[bidi] Avoid errors on uninitialized browsingContexts during command broadcast r=webdriver-reviewers,whimboo

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/aa72f80a6336

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jdescottes, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox132 to wontfix.

For more information, please visit BugBot documentation.

Comment 8

[Julian Descottes [:jdescottes]]

After getting feedback from the puppeteer folks, it seems that this patch helped with their issues on CI.

Note that they still have similar errors which I am fixing on Bug 1923899, but I don't think those are a regression from 132.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 9428331 [details]
Bug 1921756 - [bidi] Avoid errors on uninitialized browsingContexts during command broadcast

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: WebDriver BiDi commands can fail intermittently if they are sent on newly created browsing contexts. In particular this makes some commands such as the ones used for event subscription quite fragile because they attempt to reach all available browsing contexts.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is mainly adding a try catch as well as a check which will simply avoid sending a command to content processes instead of throwing. This only affects WebDriver BiDi (mostly used by Puppeteer at the moment).
• String changes made/needed:
• Is Android affected?: No

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9428331 [details]
Bug 1921756 - [bidi] Avoid errors on uninitialized browsingContexts during command broadcast

Approved for 132.0b6.

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/50828afb1490