1750623 - Crash in WebExtensionController.fromBundle(WebExtensionController.java:13): "Missing sender information."

Status: RESOLVED FIXED

(GeckoView :: Extensions, defect, P1)

Description

[Arturo Mejia [:amejia]]

We noticed some new crash reports on Sentry nightly and beta. At the moment, the volume is low, but it will be good to take a look.

java.lang.RuntimeException: Missing sender information.
    at org.mozilla.geckoview.WebExtensionController.fromBundle(WebExtensionController.java:13)
    at org.mozilla.geckoview.WebExtensionController.lambda$handleMessage$8(WebExtensionController.java:30)
    at org.mozilla.geckoview.WebExtensionController.$r8$lambda$jXq3Vt-Q1PruGyhi0zkq0ncTw50
    at org.mozilla.geckoview.WebExtensionController$$ExternalSyntheticLambda0.accept
    at org.mozilla.geckoview.GeckoResult.lambda$accept$2(GeckoResult.java:1)
    at org.mozilla.geckoview.GeckoResult.$r8$lambda$9zWUu4Bh604Sdx3PHhezEs2AuyE
    at org.mozilla.geckoview.GeckoResult$$ExternalSyntheticLambda8.onValue
    at org.mozilla.geckoview.GeckoResult.lambda$thenInternal$6(GeckoResult.java:2)
    at org.mozilla.geckoview.GeckoResult.$r8$lambda$CtmBkNQN_okuE2MBIfsimJFUi6Q
    at org.mozilla.geckoview.GeckoResult$$ExternalSyntheticLambda1.run
    at android.os.Handler.handleCallback(Handler.java:938)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loopOnce(Looper.java:226)
    at android.os.Looper.loop(Looper.java:313)
    at android.app.ActivityThread.main(ActivityThread.java:8582)
    at java.lang.reflect.Method.invoke(Method.java)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:563)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1133)

org.mozilla.geckoview.UncaughtException: java.lang.RuntimeException: Missing sender information.
    at org.mozilla.geckoview.GeckoResult.dispatchLocked(GeckoResult.java:4)
    at org.mozilla.geckoview.GeckoResult.completeExceptionally(GeckoResult.java:4)
    at org.mozilla.geckoview.GeckoResult.lambda$thenInternal$6(GeckoResult.java:10)
    at org.mozilla.geckoview.GeckoResult.$r8$lambda$CtmBkNQN_okuE2MBIfsimJFUi6Q
    at org.mozilla.geckoview.GeckoResult$$ExternalSyntheticLambda1.run
    at android.os.Handler.handleCallback(Handler.java:938)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loopOnce(Looper.java:226)
    at android.os.Looper.loop(Looper.java:313)
    at android.app.ActivityThread.main(ActivityThread.java:8582)
    at java.lang.reflect.Method.invoke(Method.java)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:563)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1133)

Comment 1

[Rob Wu [:robwu]]

The crash / error is in debug builds only, from https://searchfox.org/mozilla-central/rev/a2c26b9b49a521f4be39559ca1ca9c345a237c70/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/WebExtensionController.java#1100-1109

This is likely a regression from bug 1748158, because that removed the frameId parameter from non-tabs.
Is there anything in GeckoView that is not a tab, but where a frameId is expected? E.g. extension popups?

Comment 2

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

Set release status flags based on info from the regressing bug 1748158

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Rob Wu [:robwu] from comment #1)

Is there anything in GeckoView that is not a tab, but where a frameId is expected? E.g. extension popups?

Extension popups would fit that criteria yes. I think this might be reproducible by sending a "native" message from an extension page.

Comment 4

[Rob Wu [:robwu]]

The impact of this bug is that an extension popup (or a frame in an extension popup) cannot send a native message to the embedder any more. Native messaging is only available to built-in/privileged extensions, i.e. extensions installed by the embedder.
The debug-only mode triggers an assertion, but the non-debug version results in a null sender which prevents the message from being handled.

The diff from bug 1748158's patch that causes this assertion failure is at https://searchfox.org/mozilla-central/diff/2a62c0c6f21ded1d72dce8d655a453af2ec36641/toolkit/components/extensions/ExtensionParent.jsm#296-316
GeckoView receives that sender via GeckoViewConnection from https://searchfox.org/mozilla-central/rev/78963fe42f8d5f582f84da84a5e78377b6c1fc32/toolkit/components/extensions/ExtensionParent.jsm#280.

frameId was unintentionally exposed to non-tabs, which bug 1748158 fixed by omitting it for non-tabs, because extensions cannot do anything meaningful with the frameId without a tab. GeckoView does however require frameId to be set in its "native" message handler, with the comment

 // If session is present we are either receiving this message from a content script or
 // an extension page, let's make sure we have the proper identification so that
 // embedders can check the origin of this message.

For messages from background pages, session is null, and isToplevel is set to true without further validation of url or frameId. Would it make sense to do something similar for extension popups?

Alternatively, if the goal is to fix the regression before figuring out the next steps, then we could unconditionally add sender.frameId to getSender's return value at https://searchfox.org/mozilla-central/rev/78963fe42f8d5f582f84da84a5e78377b6c1fc32/toolkit/components/extensions/ExtensionParent.jsm#280

Note: if isTopLevel is supposed to represent whether the sender was actually the top-level frame, then the logic is flawed (and this is already the case for the existing code). A background page (and an extension popup) can contain an iframe. The iframe may contain extension code, or a website. The latter does currently not work due to bug 1724099, which enables you to fix the logic without new regressions.

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1750623 - Always set isTopLevel=true for ENV_EXTENSION senders.

isTopLevel is used for ENV_TYPE_CONTENT_SCRIPT to let embedders know if an
iframe is sending messages to the app. For extension environments we don't need
this extra check so we can always set the value to true.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Rob Wu [:robwu] from comment #4)

For messages from background pages, session is null, and isToplevel is set to true without further validation of url or frameId. Would it make sense to do something similar for extension popups?

I believe that's OK as we only need to check for isTopLevel on content scripts.

Comment 7

[Rob Wu [:robwu]]

(In reply to Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from comment #6)

(In reply to Rob Wu [:robwu] from comment #4)

For messages from background pages, session is null, and isToplevel is set to true without further validation of url or frameId. Would it make sense to do something similar for extension popups?

I believe that's OK as we only need to check for isTopLevel on content scripts.

Due to bug 1724099, it is not possible to load non-extension content in extension pages. However, once that bug is fixed, it will be possible to hit this assertion again. That logical flaw is pre-existing, so I'll not block the patch on landing (to resolve the current regression), but a follow-up is necessary in order to define meaningful behavior.

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Rob Wu [:robwu] from comment #7)

(In reply to Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from comment #6)

(In reply to Rob Wu [:robwu] from comment #4)

For messages from background pages, session is null, and isToplevel is set to true without further validation of url or frameId. Would it make sense to do something similar for extension popups?

I believe that's OK as we only need to check for isTopLevel on content scripts.

Due to bug 1724099, it is not possible to load non-extension content in extension pages. However, once that bug is fixed, it will be possible to hit this assertion again. That logical flaw is pre-existing, so I'll not block the patch on landing (to resolve the current regression), but a follow-up is necessary in order to define meaningful behavior.

Do iframes inside extension pages have access to extensions API? what's their environment type? content_child?

Comment 9

[Rob Wu [:robwu]]

moz-extension:-documents that are same-origin to the ancestor frames up to the top-level frame have access to extension APIs. addon_child is their envType.

moz-extension frames can be a child of a different origin (e.g. web pages on https) when an extension opts in by declaring the page in web_accessible_resources, and are comparable to content scripts in terms of available APIs (content_child). n theory, with full Fission support they could be fully privileged extension pages (which is what Chrome did when they introduced support for Site isolation of extensions), but we don't (at the moment).

Comment 10

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Rob Wu [:robwu] from comment #9)

moz-extension:-documents that are same-origin to the ancestor frames up to the top-level frame have access to extension APIs. addon_child is their envType.

moz-extension frames can be a child of a different origin (e.g. web pages on https) when an extension opts in by declaring the page in web_accessible_resources, and are comparable to content scripts in terms of available APIs (content_child). n theory, with full Fission support they could be fully privileged extension pages (which is what Chrome did when they introduced support for Site isolation of extensions), but we don't (at the moment).

I don't think I'm following then. How can we get into a state where the environment type is not addon_child and we don't have a frameId?

From what I understand the only way to get a content_child env type is if the page is either a child of a https page (i.e. it's a tab, you can't have an extension popup with an https page, can you?) or a content script proper, also a tab.

Could you give me an example where we would have the assertion after the patch?

Or do you mean a popup wich has a https frame which itself has a moz-extension iframe in it?

Comment 11

[Rob Wu [:robwu]]

A background page is not a tab, and can contain a https-frame (when bug 1724099). If that has a content script, then you have a content_child without frameId (because it is not a tab).

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Rob Wu [:robwu] from comment #11)

A background page is not a tab, and can contain a https-frame (when bug 1724099). If that has a content script, then you have a content_child without frameId (because it is not a tab).

I see. I think then passing frameId is probably the best solution, given all these edge cases. I understand this shouldn't be exposed in sender but could we pass it somewhere else? e.g. another parameter to openNative perhaps? https://searchfox.org/mozilla-central/rev/4615b544a0f7166233c409c619b426c4025467a7/toolkit/components/extensions/ExtensionParent.jsm#279

Comment 13

[Rob Wu [:robwu]]

That would work.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c4b0ebd4a76
Always set isTopLevel=true for ENV_EXTENSION senders. r=robwu,jonalmeida

Comment 15

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4c4b0ebd4a76

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta & Release approval when you get a chance.

Comment 17

[[ex-Mozilla] Agi Sferro | :agi]

Comment on attachment 9262150 [details]
Bug 1750623 - Always set isTopLevel=true for ENV_EXTENSION senders.

Beta/Release Uplift Approval Request

• User impact if declined: Some browser features that use extension popups with native messaging will not work.
• Is this code covered by automated tests?: Yes
• 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 one-liner allows messages to not have a frameId. There are integration tests for this code path and the test framework itself uses it to run the tests.
• String changes made/needed:

Comment 18

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9262150 [details]
Bug 1750623 - Always set isTopLevel=true for ENV_EXTENSION senders.

Approved for 98 beta 3, thanks.

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f49a0ce7912

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9262150 [details]
Bug 1750623 - Always set isTopLevel=true for ENV_EXTENSION senders.

Approved for Fenix 97.2.0.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/0d9f7b8a7c3b

Comment 22

[:Grisha Kruglov]

If I'm not mistaken, we're still seeing this crash in Nightly (e.g. see https://crash-stats.mozilla.org/signature/?product=Fenix&version=100.0a1&signature=org.mozilla.geckoview.GeckoResult%24UncaughtException%3A%20at%20org.mozilla.geckoview.GeckoResult.dispatchLocked%28GeckoResult.java%29&date=%3E%3D2022-03-08T22%3A53%3A00.000Z&date=%3C2022-03-15T22%3A53%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#aggregations and https://sentry.prod.mozaws.net/operations/firefox-nightly/issues/16874158/?query=is%3Aunresolved%20UncaughtException).

Comment 23

[[ex-Mozilla] Agi Sferro | :agi]

You're correct it's still happening, this is a Debug-only crash so it would be developers encountering this, not end users. I opened Bug 1759889 to follow up, I'll add a patch to help debugging what's going on.