1682005 - Opening multiple pages from Bookmarks and navigating through them [@ mozilla::java::RuntimeTelemetry::Proxy::DispatchHistogram ]

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Emily Kager [:ekager]]

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

Steps to reproduce

1. Make sure you are signed in with your FxA account, and have multiple bookmarks saved.
2. Go to Bookmarks, select at least 6-7 pages, and choose to open them in new tabs.
3. Go to the tabs tray and open each page, one by one. Let the pages load.

Expected behavior

The browsing should go smoothly, without issues.

Actual behavior

Fenix crashes. First, the tabs tray remains blank, and the user cannot access anything else. Fenix freezes. Force closed doesn't solve the issue every time.

Device information

• Android device: Samsung Galaxy Tab A6 (Android 5.1.1)
• Fenix version: 10/26 Nightly build

Notes

• Reproduced it 2/2. Did not reproduced the crash when opening multiple pages from History.
• Not reproducible on Xiaomi Mi8 Lite (Android 9)

Details of the crash

0e2f9fdd-44b8-4a64-8a8c-1955d37df93b
<native crash>

• Socorro: https://crash-stats.mozilla.org/report/index/bp-da124479-34dc-4670-9c7d-880c10201027

---

<native crash>

Change performed by the Move to Bugzilla add-on.

Comment 1

[Emily Kager [:ekager]]

Seems to be a rising crash https://crash-stats.mozilla.org/signature/?product=Fenix&signature=mozilla%3A%3Ajava%3A%3ARuntimeTelemetry%3A%3AProxy%3A%3ADispatchHistogram&date=%3E%3D2020-12-04T17%3A47%3A00.000Z&date=%3C2020-12-11T17%3A47%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#summary

Comment 2

[Eugen Sawin [:esawin]]

If I'm reading the crash stack correctly, then Proxy::mCtx seems to be null here which seems like an impossible scenario given that the proxy ref is only set once on startup and never reassigned or deleted.

Chris, do you see something that could trigger this, maybe a SendBatch race, within the batching code?

Comment 3

[Chris H-C :chutten]

I'm not sure how a race would make the context null, could you expand on that a bit?

Looking at this crash holistically... I don't know what Fenix's releases are like, but that crash volume graph looks to track Firefox 83 pretty closely. Looking at changes to anything in EXTRACT around that time, the closest thing seems to be that Bug 1664475 and Bug 1671729 added a lot of probes. Accumulation to bug 1664475's probes could happen particularly early and frequently, so if there is some race, those probes would be my bet on which ones would find it.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: Bug 1682005 - [1.0] Attach the telemetry proxy when attaching to the runtime to avoid premature proxy registration.

Comment 5

[Eugen Sawin [:esawin]]

I've found one, potentially unrelated, issue about the way we register the telemetry proxy.
Currently, we register/attach it on construction, where construction is triggered by enabling the appropriate runtime setting. This is not correct, since up to the instantiation of the runtime, settings may change (in theory). Patch 1 should fix that.

I don't see anything else right now that could introduce a race condition where native code ends up with an invalid proxy reference.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa9c77002e44
[1.0] Attach the telemetry proxy when attaching to the runtime to avoid premature proxy registration. r=geckoview-reviewers,agi

Comment 7

[Narcis Beleuzu [:NarcisB]]

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

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 9197754 [details]
Bug 1682005 - [1.0] Attach the telemetry proxy when attaching to the runtime to avoid premature proxy registration.

Beta/Release Uplift Approval Request

• User impact if declined: Intermittent crash on page load.
• 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: https://github.com/mozilla-mobile/fenix/issues/16203
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch only moves the attaching of the Java telemetry delegate later into the startup process to avoid a race. The fix has been confirmed on Fenix Nightly.
• String changes made/needed: None

Comment 9

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9197754 [details]
Bug 1682005 - [1.0] Attach the telemetry proxy when attaching to the runtime to avoid premature proxy registration.

Approved for 86 beta, thanks.

Comment 10

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9197754 [details]
Bug 1682005 - [1.0] Attach the telemetry proxy when attaching to the runtime to avoid premature proxy registration.

Actually, this patch landed in 86 so we shouldn't need an uplift to beta, Eugen, am I missing something? Did you mean maybe requesting an uplift to an 85 dot release?

Comment 11

[Eugen Sawin [:esawin]]

(In reply to Pascal Chevrel:pascalc from comment #10)

Comment on attachment 9197754 [details]
Bug 1682005 - [1.0] Attach the telemetry proxy when attaching to the runtime to avoid premature proxy registration.

Actually, this patch landed in 86 so we shouldn't need an uplift to beta, Eugen, am I missing something? Did you mean maybe requesting an uplift to an 85 dot release?

I'm seeing this crash as early as 83 albeit in lower volume. So uplifting to 85 seems sensible, too.

Comment 12

[Pascal Chevrel (relman team) -> :pascalc]

Eugen, please request an uplift for Release then. Thanks

Comment 13

[Eugen Sawin [:esawin]]

Comment on attachment 9197754 [details]
Bug 1682005 - [1.0] Attach the telemetry proxy when attaching to the runtime to avoid premature proxy registration.

Beta/Release Uplift Approval Request

• User impact if declined: Intermittent crash on page load.
• 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: https://github.com/mozilla-mobile/fenix/issues/16203
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch only ensures that a specific JNI object isn't attached before it is fully initialized to avoid a race. The fix has been confirmed on Fenix Nightly.
• String changes made/needed: None

Comment 14

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9197754 [details]
Bug 1682005 - [1.0] Attach the telemetry proxy when attaching to the runtime to avoid premature proxy registration.

We don't have an 85 dot release planned before 86 goes live next week. Also, I am not seeing a drop of crashes in Nightly & Beta since that patch landed.