1759560 - [Bug]: Crash when tapping a link in Privacy Possum extension popup

Status: VERIFIED FIXED

(GeckoView :: Extensions, defect, P1)

Description

[Kevin Brosnan [Ex-Mozilla]]

From github: https://github.com/mozilla-mobile/fenix/issues/24228.

Steps to reproduce

1. Install extension Privacy Possum
2. Open extension popup
3. Tap on link (for this particular extension is "Report a bug")

Expected behaviour

A new tab with desired link should open

Actual behaviour

The app crashes

Device name

HUAWEI STK-LX1

Android version

29 (REL)

Firefox release type

Firefox Nightly

Firefox version

100.0a1

Device logs

Crash report: https://crash-stats.mozilla.org/report/index/bp-9aedfb0a-a029-48d1-8e6b-c80ff0220311
Logs from ADB: https://gist.github.com/Semro/f768d0099f99f2ef2eac2bd5a389227f

Additional information

I also can reproduce it in my own extension, taping on following link: https://github.com/Semro/syncwatch/blob/e476cfbe68ffd4755eede6f12daf5870f6ccfece/plugin/popup.html#L17-L22

But I can't reproduce bug when opening links in Dark Reader extension.

Change performed by the Move to Bugzilla add-on.

Comment 1

[Chris Peterson [:cpeterson]]

Crash when using Privacy Possum, one of AMO's recommended Fenix extensions.

Comment 3

[Chris Peterson [:cpeterson]]

Increasing bug priority since this is a recommended Fenix extension.

Agi wonders why this exception still breaks when it is guarded by DEBUG. Is the user testing a debug build?

Comment 4

[semro]

How to check if I have debug build? I've download Firefox Nightly from Google Play

Attached a screenshot of my current Firefox Nightly version, where bug still reproduces. https://user-images.githubusercontent.com/3332159/159987152-6ca38510-b0d5-47c4-b0d4-c8bfc411c42c.png

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to semro from comment #4)

How to check if I have debug build? I've download Firefox Nightly from Google Play

I'm not sure how to check in the UI, but if you downloaded your Firefox Nightly from Google Play, you won't have a debug build (unless something is really messed up with our Firefox builds). To get a debug build, you would need to sideload it onto your phone from another computer.

Comment 6

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

Looks like this is our top 3 crash in Nightly now, we should do something about it.

Comment 7

[Chris Peterson [:cpeterson]]

We'd like to fix this bug in 101.

Comment 8

[[:owlish] 🦉 PST]

Attachment: Bug 1759560 - Change getBrowserData to consider popups to be browser windows and not return window id if it's not a browser window

Comment 9

[Chris Peterson [:cpeterson]]

If this fix is safe, we should consider uplifting it to Beta 100 because this is a longstanding crash triggered by a Mozilla-recommended extension.

Comment 10

[Rob Wu [:robwu]]

The issue here is that the STR results in a navigation to non-extension content in the extension panel. The implementation assumes that the extension panel only hosts extension content, which is invalid and results in a crash (assertion failure) at https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/WebExtensionController.java#1098-1112

I described a way to resolve this issue in https://bugzilla.mozilla.org/show_bug.cgi?id=1750623#c4, i.e. to unconditionally set frameId. The comment is fuller, but essentially the fix is as follows:

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 11

[Rob Wu [:robwu]]

Question: do we even want to host web content in the extension popup's top level? The URL bar is not visible, only the extension name is displayed.

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Question: do we even want to host web content in the extension popup's top level? The URL bar is not visible, only the extension name is displayed.

We have filed Bug 1760608 to consider limiting the browser action popup manifest key to only allow extension URLs and not remote URLs, which in my opinion will also means that in the long run it would not be really unlikely that we may be disallowing "navigating the action popup to a remote URL from an extension page" also on the Desktop platform.

That may be something we may want to consider while fixing this bug.

Comment 13

[Chris Peterson [:cpeterson]]

Irene plans to fix this crash in GV 102, but there will be follow-up bug that will require discussion with the Add-ons team.

Comment 14

[[:owlish] 🦉 PST]

Attachment: Bug 1759560 - Use BuildConfig.DEBUG_BUILD instead of BuildConfig.DEBUG due to an Android bug which causes BuildConfig.DEBUG to be true in release builds

Comment 15

[[:owlish] 🦉 PST]

Filed a follow-up issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1771391

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ef92e77b85b
Use BuildConfig.DEBUG_BUILD instead of BuildConfig.DEBUG due to an Android bug which causes BuildConfig.DEBUG to be true in release builds r=geckoview-reviewers,agi

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27543b11dc53
Backed out changeset 2ef92e77b85b for causing Android xpcshell timeouts.

Comment 18

[Cosmin Sabou [:CosminS]]

Backed out for causing Android 7.0 x86-64 Lite WebRender opt xpcshell timeouts.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Crunning%2Cpending%2Crunnable&searchStr=android%2Cxpc%2Clite&fromchange=2ef92e77b85b907bb7c8f570133ff77e8b32fbfd&tochange=27543b11dc5360b09ccc04b5e754033aad3a81c8&selectedTaskRun=DCUN8j8vRsq4dYWtgC0TOQ.0

Failure logs: https://treeherder.mozilla.org/logviewer?job_id=379498318&repo=autoland
https://treeherder.mozilla.org/logviewer?job_id=379502554&repo=autoland
https://treeherder.mozilla.org/logviewer?job_id=379498227&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/27543b11dc53

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/264878178380
Use BuildConfig.DEBUG_BUILD instead of BuildConfig.DEBUG due to an Android bug which causes BuildConfig.DEBUG to be true in release builds r=geckoview-reviewers,agi

Comment 20

[Noemi Erli[:noemi_erli]]

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

Comment 21

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

The patch landed in nightly and beta is affected.
:owlish, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 22

[[:owlish] 🦉 PST]

Yes, I'd uplift

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval in that case :)

Comment 24

[[:owlish] 🦉 PST]

Comment on attachment 9278381 [details]
Bug 1759560 - Use BuildConfig.DEBUG_BUILD instead of BuildConfig.DEBUG due to an Android bug which causes BuildConfig.DEBUG to be true in release builds

Beta/Release Uplift Approval Request

• User impact if declined: The app would crash if when using Privacy Possum, one of AMO's recommended Fenix extensions

• Is this code covered by automated tests?: Yes

• Has the fix been verified in Nightly?: No

• Needs manual test from QE?: Yes

• If yes, steps to reproduce: Steps to reproduce:

  1. Install extension Privacy Possum
  2. Open extension popup
  3. Tap on link (for this particular extension is "Report a bug")

Expected behaviour: A new tab with desired link should open

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change affects checks for a debug build, so it doesn't affect browser functionality - most of the time it would be conditional logging and crashing.
• String changes made/needed:
• Is Android affected?: Yes

Comment 25

[Pascal Chevrel:pascalc]

Comment on attachment 9278381 [details]
Bug 1759560 - Use BuildConfig.DEBUG_BUILD instead of BuildConfig.DEBUG due to an Android bug which causes BuildConfig.DEBUG to be true in release builds

Approved for 102 beta 6, thanks.

Comment 26

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/6812cb4c2ec5

Comment 27

[Delia Pop]

Verified as fixed on the latest Nightly 103.0a1 from 06/10 with Google Pixel 6 (Android 12). The GitHub web page opens and the application remains stable after tapping the "Report a bug" link from the add-on screen.