1346042 - Intermittent dom/media/test/test_invalid_reject_play.html | application crashed [@ 0xe7574c24][@ LibHandle::ReleaseDirectRef]

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Treeherder Bug Filer]

Filed by: wkocher [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=82772935&repo=autoland

https://autophone.s3.amazonaws.com/v1/task/Xgvgyx6ATOyu71dKRauLxg/runs/0/artifacts/public/build/mochitest-dom-media-mochitests-dom-media-settings.ini-1-pixel-07-d7c18e37-0f10-43cd-b1f0-52689666c650.log

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

https://treeherder.mozilla.org/logviewer.html#?job_id=82772935&repo=autoland&lineNumber=3579
Assertion failure: directRefCnt <= mozilla::external::AtomicRefCounted<LibHandle>::refCount(), at /home/worker/workspace/build/src/mozglue/linker/ElfLoader.h:167

Comment 2

[Intermittent Failures Robot]

6 failures in 790 pushes (0.008 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 3
* mozilla-inbound: 2
* mozilla-central: 1

Platform breakdown:
* android-7-1-armv8-api15: 5
* android-6-0-armv8-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1346042&startday=2017-03-06&endday=2017-03-12&tree=all

Comment 3

[Intermittent Failures Robot]

8 failures in 777 pushes (0.01 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 4
* mozilla-central: 2
* autoland: 2

Platform breakdown:
* android-7-1-armv8-api15: 8

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1346042&startday=2017-03-13&endday=2017-03-19&tree=all

Comment 4

[Intermittent Failures Robot]

5 failures in 898 pushes (0.006 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 3
* mozilla-inbound: 2

Platform breakdown:
* android-7-1-armv8-api15: 5

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1346042&startday=2017-03-20&endday=2017-03-26&tree=all

Comment 5

[Intermittent Failures Robot]

14 failures in 845 pushes (0.017 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 9
* mozilla-inbound: 4
* mozilla-central: 1

Platform breakdown:
* android-7-1-armv8-api15: 13
* android-6-0-armv8-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1346042&startday=2017-03-27&endday=2017-04-02&tree=all

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1346042-1.0-Mutex-lock-LibHandle-direct-referenc.patch

I can't reproduce this locally with the test page, but the assertion should only fail when we're calling AddDirectRef and ReleaseDirectRef simultaneously on different threads.

Given that it's a low-frequency failure (1:100), I'm not sure how insightful this test run is, but it looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ef201ca0182c90ee5e2f53323c103cbe6ff6d3c

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8855943 [details] [diff] [review]
0001-Bug-1346042-1.0-Mutex-lock-LibHandle-direct-referenc.patch

Review of attachment 8855943 [details] [diff] [review]:
-----------------------------------------------------------------

Haven't thought much about it, but shouldn't making that refcounter atomic be enough?

Comment 8

[Eugen Sawin [:esawin]]

(In reply to Mike Hommey [:glandium] from comment #7)
> Haven't thought much about it, but shouldn't making that refcounter atomic
> be enough?

We can check the assertion in ReleaseDirectRef() on a different thread after the directRefCnt increment but before calling AddRef() in AddDirectRef(). I think we need to lock this down to ensure correctness.

The chances for hitting this window are so slim that I expect that something else might be going wrong, but nonetheless, we should make the custom ref-counting thread-safe.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3035c59fe9
[1.0] Mutex-lock LibHandle direct reference management. r=glandium

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b3035c59fe9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem high enough volume to worry about backporting to 54, but feel free to set the flag back to affected and request uplift if you feel otherwise.

Comment 12

[Eugen Sawin [:esawin]]

Comment on attachment 8855943 [details] [diff] [review]
0001-Bug-1346042-1.0-Mutex-lock-LibHandle-direct-referenc.patch

Approval Request Comment
[Feature/Bug causing the regression]: N/A.
[User impact if declined]: Crash on startup.
[Is this code covered by automated tests?]: Only indirectly through app startup.
[Has the fix been verified in Nightly?]: Yes, no crashes with this signature in automation.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: 1358241, 1361903.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Affects only locking mechanics during startup.
[String changes made/needed]: None.

Comment 13

[Julien Cristau [:jcristau]]

(In reply to Eugen Sawin [:esawin] from comment #12)
> Comment on attachment 8855943 [details] [diff] [review]
> 0001-Bug-1346042-1.0-Mutex-lock-LibHandle-direct-referenc.patch
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: N/A.
> [User impact if declined]: Crash on startup.
> [Is this code covered by automated tests?]: Only indirectly through app
> startup.
> [Has the fix been verified in Nightly?]: Yes, no crashes with this signature
> in automation.
> [Needs manual test from QE? If yes, steps to reproduce]: No.
> [List of other uplifts needed for the feature/fix]: 1358241, 1361903.
> [Is the change risky?]: No.
> [Why is the change risky/not risky?]: Affects only locking mechanics during
> startup.
> [String changes made/needed]: None.

There's no uplift request on the bugs you list as prerequisites, is that an oversight?  Is there any evidence of this being a high-volume issue for users?  We're a week away from building release candidates for 54 so unless that's the case I think it's too late to take this change.

Comment 14

[Eugen Sawin [:esawin]]

(In reply to Julien Cristau [:jcristau] from comment #13)
> There's no uplift request on the bugs you list as prerequisites, is that an
> oversight?

I assumed listing the blocking bugs in the request takes care of it.
I can request for uplift individually if that's not the case.

> Is there any evidence of this being a high-volume issue for
> users?  We're a week away from building release candidates for 54 so unless
> that's the case I think it's too late to take this change.

This does not seem to be a high-volume issue in production, we only see this in test automation due to the assertions.

The reason for the uplift request is to remove any known linker-related issues to improve the results of LG's MTBF testing (e.g., bug 1349099) and to help isolate currently unknown issues.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8855943 [details] [diff] [review]
0001-Bug-1346042-1.0-Mutex-lock-LibHandle-direct-referenc.patch

I agree with Julien's assessment. Snorp, is this something that is a must for 54 in terms of a partner ask? I am not sure this uplift right before we enter RC week is justified.

Comment 16

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

It could help with the partner, but no reason to rush it out, and I don't think we have high confidence that it will fix their issue.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Snorp, we will let this fix ride the 55 train then.