Closed Bug 1188590 Opened 4 years ago Closed 4 years ago
_Get Global Class Ref goes reentrant on main
1.16 KB, patch
|Details | Diff | Splinter Review|
3.14 KB, patch
|Details | Diff | Splinter Review|
This has only been observed on android 4.0. (And only on mozilla-beta, since the android 4.0 tests are disabled on aurora and central) The patch from bug 1182289 detected this problem.
Ok, diagnostics are in: http://firstname.lastname@example.org/try-android-api-11-debug/try_panda_android-debug_test-mochitest-5-bm89-tests1-panda-build1491.txt.gz We are going reentrant in jsjni_GetGlobalClassRef, which is definitely android specific. https://dxr.mozilla.org/mozilla-central/source/widget/android/AndroidJNIWrapper.cpp#65
Summary: Unknown reentrancy on main somewhere below CreateOrUpdateMediaPipeline → jsjni_GetGlobalClassRef goes reentrant on main
This might work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb6def2e8aaf
Assignee: nobody → docfaraday
Ok, fixed the compile bug, looks good (there's a known perma-orange on android, but that's about it). https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b584fd6ab3f
Attachment #8640498 - Flags: review?(snorp) → review+
Comment on attachment 8640503 [details] [diff] [review] (Beta only) Reenable mochitests for bug 1013809 Approval Request Comment Just re-enabling some tests that were disabled to allow bug 1182289 to land, now that we've found the cause of the mystery reentrancy.
Comment on attachment 8640498 [details] [diff] [review] Don't sync dispatch if we're already on main Approval Request Comment for aurora, beta, and esr38 [Feature/regressing bug #]: Bug 839836 [User impact if declined]: Content will probably be able to cause UAFs in webrtc code (and probably other code too) on android. [Describe test coverage new/current, TreeHerder]: The patches from bug 1182289 uncovered this bug, because there are assertions that are tripped when main goes reentrant if CreateOrUpdateMediaPipeline is on the stack. These asserts do not cover all of the API in PeerConnection, nor do they cover other code that cannot tolerate main going reentrant. [Risks and why]: This patch alters timing (GetGlobalClassRef will finish earlier than stuff in main's event queue), so it is possible it could uncover other problems. However, I'm not seeing evidence of this on try. [String/UUID change made/needed]: None. [Security approval request comment] How easily could an exploit be constructed based on the patch? Since our mochitests for bug 1013809 hit this every time on android 4.0, it would seem pretty easy to hit this reliably. There are probably other ways to cause this bug too. It is hard to say how easy it would be to actually exploit. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The patch for re-enabling the tests for bug 1013809 on beta would provide a pretty good hint at how to trigger this bug, but the fix itself does not paint this kind of bullseye. We may wish to open a separate bug for the test patch, so the two are harder to link together. Which older supported branches are affected by this flaw? All. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I'm pretty sure the patch will apply cleanly. If not, it will be a trivial backport. How likely is this patch to cause regressions; how much testing does it need? This patch alters timing (GetGlobalClassRef will finish earlier than stuff in main's event queue), so it is possible it could uncover other problems. However, I'm not seeing evidence of this on try.
Comment on attachment 8640498 [details] [diff] [review] Don't sync dispatch if we're already on main Android isn't supported on esr38
Comment on attachment 8640498 [details] [diff] [review] Don't sync dispatch if we're already on main Approvals given!
Comment on attachment 8640503 [details] [diff] [review] (Beta only) Reenable mochitests for bug 1013809 Test-only changes can land w/o approval.
https://hg.mozilla.org/releases/mozilla-aurora/rev/0104fc2803cb https://hg.mozilla.org/releases/mozilla-beta/rev/d87f96fc45b1 https://hg.mozilla.org/releases/mozilla-beta/rev/6b716448c59d Calling this in-testsuite+ since we in fact have tests that caught the issue, albeit on beta and on a platform we're soon to not be testing on anymore.
Attachment #8640503 - Flags: review?(rjesup) → review+
backlog: --- → webRTC+
Priority: -- → P1
See bug 1189058. The issue there was detected on the media thread. This bug will fix it for the main thread (and is a good idea) but it doesn't fix the underlying cause.
Whiteboard: [adv-main40+] → [adv-main40+][post-critsmash-triage]
I'm very worried about the fact that this was only caught by chance on an uplift to beta on a platform that's soon to be decommissioned. Looking at the test manifest, we disable all WebRTC tests on Android 2.3/4.3 - http://hg.mozilla.org/mozilla-central/file/e9389ca320ff/dom/media/tests/mochitest/mochitest.ini#l4. Looks like this goes back to when 2.3/4.3 were enabled to begin with. Maire, any idea what needs to be done to get these running again on 41+? We obviously got lucky this time.
I've opened a new bug and made it a P1: Bug 1189784. I gave Catlee a heads up that we'd likely need someone from releng to assist, and kmoir has already pinged me to ask how she can help.
You need to log in before you can comment on or make changes to this bug.