1846343 - TabManager.getNavigableForBrowsingContext() fails when passed in browsing context is null

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P2)

Description

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

We immediately check for browsingContext.isContent but do no null check before.

https://searchfox.org/mozilla-central/rev/e4fa79ba05364da0d6ec40d3f9dbe0d15ce6f902/remote/shared/TabManager.sys.mjs#267-270

Most likely we want to return null here as well when the browsing context is null.

Comment 1

[Julian Descottes [:jdescottes]]

Note that call sites will have to handle null anyway. Either they should check the browsing context before calling getNavigableForBrowsingContext or they need to null check the returned navigable.

I think I would rather keep failing in getNavigableForBrowsingContext, maybe with a clearer failure. For instance, when looking at the NavigationManager, returning null might lead to unexpected side effects. Eg we can end up using null as a key in our internal map, which might make things hard to investigate.

Comment 2

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

We have other APIs like getBrowserForTab(), getTabBrowser(), getBrowserById, and others that return null if the related argument is null as well. So maybe we should keep all these APIs in sync and return null here as well and not a failure?

Comment 3

[Julian Descottes [:jdescottes]]

Looking a bit at the APIs listed above:

• getTabBrowser will throw if win is null
• getBrowserById is a different kind of API, we never null check id, but a null id will be treated similarly to passing an outdated id.

getIdForBrowsingContext and getIdForBrowser consistently check against null, but that leads to bugs such as https://bugzilla.mozilla.org/show_bug.cgi?id=1847563

I don't really have a strong opinion, as long as we make sure to update the callsites to handle the null or error case properly.

Comment 4

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

Right. Does it mean this bug blocks bug 1847563? I'm happy to provide a fix here so that we can better check for a non-existent navigable.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #4)

Right. Does it mean this bug blocks bug 1847563? I'm happy to provide a fix here so that we can better check for a non-existent navigable.

Yes, it sounds fine to block. We can apply whatever pattern we use here for getNavigableForBrowsingContext to Bug 1847563 and getIdForBrowsingContext. (or we can also review all TabManager APIs and all consumers in this bug)

Comment 6

[Julian Descottes [:jdescottes]]

As discussed, let's focus on getNavigableForBrowsingContext here, and file follow up bugs for other APIs in order to apply a similar pattern.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1846343 - [remote] Throw an error when getNavigableForBrowsingContext() is called with an invalid context

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1846343 - [remote] Check for valid browsing context in NavigationManager getNavigationForBrowsingContext

Depends on D187476

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1846343 - [remote] Do not emit network events for discarded browsing contexts

Depends on D187477

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1846343 - [remote] Check for valid browsing context in Session getSeenNodesForBrowsingContext

Depends on D187478

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1846343 - [remote] Catch type errors for navigation messages from child actors

Depends on D187479

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ccc87553d19
[remote] Throw an error when getNavigableForBrowsingContext() is called with an invalid context r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/4875bf7eeb5b
[remote] Check for valid browsing context in NavigationManager getNavigationForBrowsingContext r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/f03aef4971b2
[remote] Do not emit network events for discarded browsing contexts r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/128da1e1606f
[remote] Check for valid browsing context in Session getSeenNodesForBrowsingContext r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/2e2b8c5f6818
[remote] Catch type errors for navigation messages from child actors r=webdriver-reviewers,whimboo

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/5ccc87553d19
https://hg.mozilla.org/mozilla-central/rev/4875bf7eeb5b
https://hg.mozilla.org/mozilla-central/rev/f03aef4971b2
https://hg.mozilla.org/mozilla-central/rev/128da1e1606f
https://hg.mozilla.org/mozilla-central/rev/2e2b8c5f6818