1482452 - [Android Pie] Crash in libart.so@0x34db51

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-cc050cad-d3ec-4a10-81e1-b582a0180804.
=============================================================

Seen while looking at Fennec crash stats: https://bit.ly/2OZNkbc. Devices affected so far are Pixel 2, Pixel 2 XL and Pixel XL. 27 crashes across all versions in the last week.

Top 10 frames of crashing thread:

0 libc.so libc.so@0x1cd66 
1 libart.so libart.so@0x34db51 
2 libbase.so libbase.so@0x71b3 
3 libart.so libart.so@0x372b7d 
4 boot-framework.vdex boot-framework.vdex@0xae297a 
5 base.vdex base.vdex@0x58be88 
6 libart.so libart.so@0xdc1cf 
7 libart.so libart.so@0x45d4bf 
8 libart.so libart.so@0x44581c 
9 libc++.so libc++.so@0x4389b 

=============================================================

Comment 1

[Marcia Knous [:marcia]]

For 61.8% of the users, this is a startup crash. Pixel XL has the highest number of crashes.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Low volume crash at this point.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

This looks like decoder stuff, John.

Comment 4

[John Lin [:jhlin][:jolin]]

The majority of crashes are either from HLSTrackDemuxer::UpdateMediaInfo() or Remote(Video|Audio)Decoder::Init() while accessing Java runtime through JNI.

For UpdateMediaInfo(), the stack shows that it was trying to access codec specific data buffer[1], which could be null because of errors in exoplayer parser or malformed stream and causes trouble. (Though it cannot explain why this only happens in Pie.) We can improve the code by checking nullness/emptiness here.

For Init(), the stack shows it crashed in jni::GetNativeHandle()[2] and by inspecting the code, it seems to me that the only reason thing could go wrong is again null jobject. Since constructing a new Java object is fallable, we'd better check it and return error here too.

[1] https://searchfox.org/mozilla-central/source/dom/media/hls/HLSDemuxer.cpp#359
[2] https://searchfox.org/mozilla-central/source/widget/android/jni/Utils.cpp#269

Comment 5

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1482452 - p1: validate buffer contents before accessing. r?snorp!

Comment 6

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1482452 - p2: validate object before sending to Java runtime. r?snorp!

Depends on D16707

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0be55928fcb7
p1: validate buffer contents before accessing. r=snorp
https://hg.mozilla.org/integration/autoland/rev/ba98166b6967
p2: validate object before sending to Java runtime. r=snorp

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/0be55928fcb7
https://hg.mozilla.org/mozilla-central/rev/ba98166b6967

Comment 9

[Ryan VanderMeulen [:RyanVM]]

John, is this worth uplifting to 65 still? It grafts cleanly as-landed. Crash volume on release is pretty low, but the patches don't look particularly scary either. Please nominate the patches for Beta approval if you think they're safe to take.

Comment 10

[John Lin [:jhlin][:jolin]]

Comment on attachment 9036955 [details]
Bug 1482452 - p2: validate object before sending to Java runtime. r?snorp!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: 1257777 & 1380237

User impact if declined: browser crash.

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: p1 (D16707)

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The changes add nullness checks.

String changes made/needed: None

Comment 11

[John Lin [:jhlin][:jolin]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

John, is this worth uplifting to 65 still? It grafts cleanly as-landed. Crash volume on release is pretty low, but the patches don't look particularly scary either. Please nominate the patches for Beta approval if you think they're safe to take.

Uplift requested. Thanks, Ryan!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9036955 [details]
Bug 1482452 - p2: validate object before sending to Java runtime. r?snorp!

[Triage Comment]
Adds some null checks to avoid crashes. Approved for Fennec 65.0 RC1.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/288c5869de06
https://hg.mozilla.org/releases/mozilla-release/rev/83c4dc89cf3d

Comment 14

[miralobontiu]

Hi,

We tested on Google Pixel 3XL (Android P), Google Pixel (Android P), and Google Pixel XL (Android P), on the latest Nightly 66.0a1 (2019-01-22) an on RC 65.0, and we didn`t have a crash.
Since there are no exact steps, we tried restarting Firefox multiple times, open multiple apps, open multiple tabs, played multiple videos.

I will not update the status to "verified", because of the missing STR.

Thank you!