Closed
Bug 1162720
Opened 9 years ago
Closed 9 years ago
enumerateDevices crashes on Android (webcamtoy.com crashes)
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: jib, Assigned: jib)
References
Details
(Keywords: crash)
Crash Data
Attachments
(4 files, 15 obsolete files)
1.47 KB,
patch
|
jib
:
review+
Sylvestre
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
18.74 KB,
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
2.66 KB,
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-b779b5df-20d5-4880-8397-f4d8c2150507. ============================================================= STR: 1. On Fennec, go to http://jsfiddle.net/jib1/Lqo4paed 2. Push [Enumerate] button. 3. crashes Similarly crashes when getting camera in http://webcamtoy.com > 0 libxul.so nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJS.cpp > 1 libxul.so PrepareAndDispatch xpcom/reflect/xptcall/md/unix/xptcstubs_arm.cpp > 2 libxul.so NS_InvokeByIndex > 3 libxul.so FindProviderFile xpcom/io/nsDirectoryService.cpp > 4 libxul.so nsDirectoryService::Get(char const*, nsID const&, void**) xpcom/io/nsDirectoryService.cpp > 5 libxul.so mozilla::media::ParentSingleton::OriginKeysLoader::GetFile() xpcom/io/nsDirectoryServiceUtils.h > 6 libxul.so mozilla::media::ParentSingleton::OriginKeysLoader::Read() dom/media/systemservices/MediaParent.cpp > 7 libxul.so mozilla::media::Parent::Parent() dom/media/systemservices/MediaParent.cpp > 8 libxul.so mozilla::media::AllocPMediaParent() dom/media/systemservices/MediaParent.cpp I've verified that the crash dates back to 2015-03-31 [1] when enumerateDevices was added. Looks like poor testing on my part (while I recall seeing successful tests on Android during development, I clearly must not have tested the final e10s version) :-( It seems to fail to get nsDirectoryService on a main process background thread: http://mxr.mozilla.org/mozilla-central/source/dom/media/systemservices/MediaParent.cpp?rev=e58ede36707b#128 [1] https://crash-stats.mozilla.com/report/index/4424d39d-0a7b-44d0-856b-078f12150507
Assignee | ||
Comment 1•9 years ago
|
||
Temporarily omit code that persists to profile dir on fennec. I'm running tries with and without this patch - with: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16b634cf595a - wout: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bab1eaa27eb9
Assignee | ||
Updated•9 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 2•9 years ago
|
||
Comments in Bug 1161870 seem to confirm the crash on B2G as well.
Summary: enumerateDevices crashes on Fennec (webcamtoy.com crashes) → enumerateDevices crashes on Android (webcamtoy.com crashes)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #1) > I'm running tries with and without this patch Here are better tries (mochitest 5 is where it's at nowadays, not 6): with: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8194a223732 wout: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dd542332775 These show that the patch takes care of the crash (by skipping the offending code).
Assignee | ||
Comment 4•9 years ago
|
||
And M 13 on b2g.
Assignee | ||
Comment 5•9 years ago
|
||
Turn off on B2G as well as the problem shows there.
Attachment #8602988 -
Attachment is obsolete: true
Attachment #8603101 -
Flags: review?(rjesup)
Assignee | ||
Comment 6•9 years ago
|
||
Fix orange by loosening test on label (permissions seem to vary in different test environments).
Attachment #8602986 -
Attachment is obsolete: true
Attachment #8603102 -
Flags: review?(rjesup)
Assignee | ||
Comment 7•9 years ago
|
||
Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a7191151770
Comment 8•9 years ago
|
||
Comment on attachment 8603101 [details] [diff] [review] Quick-fix: Turn off persisting of enumerateDevice ids on android (2) Review of attachment 8603101 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/systemservices/MediaParent.cpp @@ +281,5 @@ > > nsresult Load() > { > +#if defined(ANDROID) > + return NS_OK; // Bug XXX fill in the XXX's @@ +309,5 @@ > void Clear(int64_t aSinceWhen) > { > OriginKeysTable::Clear(aSinceWhen); > +#if defined(ANDROID) > +#else ummmm. Either !defined, or put something there
Attachment #8603101 -
Flags: review?(rjesup) → review+
Updated•9 years ago
|
Attachment #8603102 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Update comment and #if. Carrying forward r=jesup.
Attachment #8603101 -
Attachment is obsolete: true
Attachment #8603331 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
Update commit msg.
Assignee: nobody → jib
Attachment #8603331 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8603332 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8603102 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8603331 -
Attachment is obsolete: false
Assignee | ||
Comment 11•9 years ago
|
||
We can land and uplift part 1 to stop the crashing, while figuring out here what the problem is exactly with writing to the profile-dir on android off main thread.
Keywords: checkin-needed,
leave-open
Assignee | ||
Comment 12•9 years ago
|
||
Turns out NS_GetSpecialDirectory is main-thread only (see Bug 1163021). Updating code to pass from main.
Assignee | ||
Comment 13•9 years ago
|
||
Quick caching-fix [1] didn't work for some reason. Need more time to add passing to main and back (since no main is involved at present in MediaParent). I suggest landing the crash-fix patches that are here for now (maybe uplift to 39 before merge?) - Those will turn off persistence across restart on android, but otherwise will let enumerateDevices work within a browser session there. Already marked as checkin-needed. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e151f147ef
Assignee | ||
Comment 14•9 years ago
|
||
Unbitrot.
Attachment #8603331 -
Attachment is obsolete: true
Attachment #8603332 -
Attachment is obsolete: true
Attachment #8604437 -
Flags: review+
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8604439 -
Flags: review?(rjesup)
Assignee | ||
Updated•9 years ago
|
Attachment #8604439 -
Attachment description: rename CallbackRunnable::New to NewRunnableFrom → Part 2: rename CallbackRunnable::New to NewRunnableFrom
Assignee | ||
Comment 16•9 years ago
|
||
Working fix. Try is closed, so here's an earlier one that works on android (compile problem on Windows should be fixed): https://treeherder.mozilla.org/#/jobs?repo=try&revision=2119cea6aa95
Attachment #8604441 -
Flags: review?(rjesup)
Assignee | ||
Comment 17•9 years ago
|
||
Green try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a87cbfafde1
Updated•9 years ago
|
Attachment #8604439 -
Flags: review?(rjesup) → review+
Updated•9 years ago
|
Attachment #8604441 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Update commit msg.
Attachment #8604437 -
Attachment is obsolete: true
Attachment #8605797 -
Flags: review+
Assignee | ||
Comment 19•9 years ago
|
||
Update commit msg.
Attachment #8604439 -
Attachment is obsolete: true
Attachment #8605798 -
Flags: review+
Assignee | ||
Comment 20•9 years ago
|
||
Update commit msg.
Attachment #8604441 -
Attachment is obsolete: true
Attachment #8605799 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 22•9 years ago
|
||
Sure, though I was hoping someone would land it on m-i first. ;-)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed,
crash
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed,
crash
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2f391a7fdd https://hg.mozilla.org/integration/mozilla-inbound/rev/94a7098042fb https://hg.mozilla.org/integration/mozilla-inbound/rev/05306872093a
Keywords: checkin-needed
Comment 24•9 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e19e46655f8c for consistently crashing b2g debug, https://treeherder.mozilla.org/logviewer.html#?job_id=9903698&repo=mozilla-inbound, and occasionally managing to crash other platforms, https://treeherder.mozilla.org/logviewer.html#?job_id=9908091&repo=mozilla-inbound and perhaps https://treeherder.mozilla.org/logviewer.html#?job_id=9906466&repo=mozilla-inbound, dunno about that last one.
Assignee | ||
Comment 25•9 years ago
|
||
Had to make media::Parent (which is held alive by the media::Pledge's .Then() handler) use thread-safe ref-counting, since the runnable ends up being freed on the other thread. Works: Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d88865b5193 After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc4b03766d94 I'm quite unhappy about this fix, since requiring every .Then() handler to be threadsafe is surprising and seems unnecessary when it always executes on the same thread. I have another (a bit more elaborate) solution in mind, so uploading this for now.
Attachment #8605799 -
Attachment is obsolete: true
Assignee | ||
Comment 26•9 years ago
|
||
Preferred solution. - Repurposed coat-check from media::Child in media::Parent as well to avoid requiring thread-safe refcounting of Pledge.Then() lambdas, by sending ids instead. Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=50883d30d591
Attachment #8607169 -
Attachment is obsolete: true
Attachment #8607208 -
Flags: review?(rjesup)
Comment 27•9 years ago
|
||
Comment on attachment 8607208 [details] [diff] [review] Part 3: enumerateDevices visits main thread for profileDir (4) Review of attachment 8607208 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/systemservices/MediaUtils.h @@ +137,5 @@ > { > return new LambdaRunnable<OnRunType>(aOnRun); > } > > +// General coat-check (for pledges, or anything. Optimized for very small sets) Expand the comment so other people looking at MediaUtils will understand what this provides them and when/how to use it. Think of the comments in mfbt/*, though this doesn't have to be as extensive. Document thread-safety issues Also disallow copy/assign I think @@ +143,5 @@ > +template<class T> > +class CoatCheck > +{ > +public: > + typedef std::pair<uint32_t,nsRefPtr<T>> Element; space after , @@ +165,5 @@ > + } > + MOZ_ASSERT_UNREACHABLE("Received response with no matching media::ChildPledge!"); > + return nullptr; > + } > +private: nit: blank line before private @@ +171,5 @@ > + { > + static uint32_t counter = 0; > + return ++counter; > + }; > + nsTArray<Element> mElements; Perhaps nsAutoTArray<Element, 1> (or some small number) This and the static counter require that both Append and Remove be used from a single thread. Consider adding some debug-only checking of that, but at minimum document this requirement.
Attachment #8607208 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 28•9 years ago
|
||
Incorporate feedback. Carrying forward r=jesup. - Added mfbt/* style comments. - nsAutoTArray<Element, 3> - whitespace
Attachment #8607208 -
Attachment is obsolete: true
Attachment #8608309 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 29•9 years ago
|
||
Minor comment tweak. Carrying forward r=jesup.
Attachment #8608309 -
Attachment is obsolete: true
Attachment #8608334 -
Flags: review+
Assignee | ||
Comment 30•9 years ago
|
||
Another minor comment tweak, sorry. Carrying forward r=jesup.
Attachment #8608334 -
Attachment is obsolete: true
Attachment #8608335 -
Flags: review+
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b76e306cbd https://hg.mozilla.org/integration/mozilla-inbound/rev/c70d228147df https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed581fdb25b
Keywords: checkin-needed
Comment 32•9 years ago
|
||
Backed out for mochitest failures. https://treeherder.mozilla.org/logviewer.html#?job_id=10041547&repo=mozilla-inbound
Comment 33•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f2547a9e05
Assignee | ||
Comment 34•9 years ago
|
||
Updated to work with new gum test harness (Bug 1161615). Carrying forward r=jesup. Green try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=1793956c4756
Attachment #8605797 -
Attachment is obsolete: true
Attachment #8609063 -
Flags: review?(drno)
Comment 35•9 years ago
|
||
Comment on attachment 8609063 [details] [diff] [review] Part 1: test of enumerateDevices (6) r=jesup Review of attachment 8609063 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/test_enumerateDevices.html @@ +16,5 @@ > + ok(devices.length > 0, "At least one device found"); > + devices.forEach(d => { > + ok(d.kind == "videoinput" || d.kind == "audioinput", "Known device kind"); > + is(d.deviceId.length, 44, "Correct device id length"); > + ok(d.label.length !== undefined, "Device label: " + d.label); What about d.label.length == 0?
Comment 36•9 years ago
|
||
Comment on attachment 8609063 [details] [diff] [review] Part 1: test of enumerateDevices (6) r=jesup Review of attachment 8609063 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, the additional label length check is optional :-)
Attachment #8609063 -
Flags: review?(drno) → review+
Assignee | ||
Comment 37•9 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #35) > What about d.label.length == 0? Would fail because permission is disabled on the tree: TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_enumerateDevices.html | Device label is empty: FaceTime HD Camera (Built-in) - got 29, expected 0
Assignee | ||
Comment 38•9 years ago
|
||
Update commit msg.
Attachment #8609063 -
Attachment is obsolete: true
Attachment #8609109 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 39•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6472e42af0c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/5c651a72987c https://hg.mozilla.org/integration/mozilla-inbound/rev/547838f45357
Keywords: checkin-needed
Comment 41•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6472e42af0c7 https://hg.mozilla.org/mozilla-central/rev/5c651a72987c https://hg.mozilla.org/mozilla-central/rev/547838f45357
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 42•9 years ago
|
||
I've just hit this exact crash on Android on FF 39 Beta, testing some Hello updates. Can we get it uplifted?
Flags: needinfo?(mreavy)
Flags: needinfo?(jib)
Comment 43•9 years ago
|
||
I didn't think the base bug was on 39 (though maybe 40) - jib, please check
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8603101 [details] [diff] [review] Quick-fix: Turn off persisting of enumerateDevice ids on android (2) Ugh, forgot to follow up and uplift this. my bad. :-( Also didn't that I obsoleted this quick-fix patch. Options: Do we want this quick-fix? Or do we prefer the final fix which involves a main-thread visit, but arguably has been on m-c and aurora longer?
Attachment #8603101 -
Attachment description: Part 1: Turn off persisting of enumerateDevice ids on android (2) → Quick-fix: Turn off persisting of enumerateDevice ids on android (2)
Attachment #8603101 -
Attachment is obsolete: false
Assignee | ||
Comment 45•9 years ago
|
||
Comment on attachment 8603101 [details] [diff] [review] Quick-fix: Turn off persisting of enumerateDevice ids on android (2) Approval Request Comment [Feature/regressing bug #]: Bug 1046245 [User impact if declined]: Web pages that call mediaDevices.enumerateDevices crash on android. [Describe test coverage new/current, TreeHerder]: Quick fix has been tested locally and landed on m-c a while ago. Independent verification still pending in Bug 1161870 comment 14. I've just reached out about that. Quick-fix has since been replaced by proper fix. [Risks and why]: Very low. Patch disables part of code that crashed. If we take this patch then the anonymized deviceIds wont persist from run to run on android. In other words, web pages will get unique ones after a restart (like they already get in private browsing mode). Since the deviceId constraint isn't finished yet, this shouldn't matter terribly. Desktop is not affected by this patch (desktop never crashed). [String/UUID change made/needed]: None
Attachment #8603101 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 46•9 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #45) > Independent verification still pending in Bug 1161870 comment 14. I should note that any verification at this point would end up being of the proper fix rather than the quick-fix, unless a previous nightly was pulled for testing.
Flags: needinfo?(jib)
Assignee | ||
Comment 47•9 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #45) > [Describe test coverage new/current, TreeHerder]: > Quick fix has been tested locally and landed on m-c a while ago. Correction, quick-fix never landed on m-c, as it sat for a week with checkin-needed and was overtaken by events, the real fix (possibly due to misinterpreting comment 13 where I talk about a separate 'quick' solution to get persisting working again that didn't work?)
Comment 48•9 years ago
|
||
Jib, Standard8 and I talked on irc. We need the "quick fix" uplifted to fx39 and fx40; only fennec is affected. Jib will set the right approval flags and provide additional details.
Flags: needinfo?(mreavy)
Assignee | ||
Comment 49•9 years ago
|
||
Comment on attachment 8603101 [details] [diff] [review] Quick-fix: Turn off persisting of enumerateDevice ids on android (2) Approval Request Comment [Feature/regressing bug #]: Bug 1046245 [User impact if declined]: Web pages that call mediaDevices.enumerateDevices crash on android. [Describe test coverage new/current, TreeHerder]: Quick fix has been tested locally, but never landed on m-c, possibly due to a confusing comment 13. Note that we don't want to land this quick-fix patch on 41, since m-c already has the proper fix. [Risks and why]: Very low. Patch disables on android the part of the code that crashed on android. If we take this patch then the anonymized deviceIds wont persist from run to run on android. In other words, web pages will get unique ones after a restart (like they already get in private browsing mode). Since the deviceId constraint isn't finished yet, this shouldn't matter terribly. Desktop is not affected by this patch (desktop never crashed). [String/UUID change made/needed]: None
Attachment #8603101 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 50•9 years ago
|
||
Comment on attachment 8603101 [details] [diff] [review] Quick-fix: Turn off persisting of enumerateDevice ids on android (2) Ugh, dug out wrong patch, sorry. busy morning.
Attachment #8603101 -
Attachment is obsolete: true
Attachment #8603101 -
Flags: approval-mozilla-beta?
Attachment #8603101 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 51•9 years ago
|
||
Comment on attachment 8603331 [details] [diff] [review] Quick-fix: Turn off persisting of enumerateDevice ids on android (3) r=jesup This is the right patch with review-nits addressed. Approval Request Comment [Feature/regressing bug #]: Bug 1046245 [User impact if declined]: Web pages that call mediaDevices.enumerateDevices crash on android. [Describe test coverage new/current, TreeHerder]: Quick fix has been tested locally, but never landed on m-c, possibly due to a confusing comment 13. Note that we don't want to land this quick-fix patch on 41, since m-c already has the proper fix. [Risks and why]: Very low. Patch disables on android the part of the code that crashed on android. If we take this patch then the anonymized deviceIds wont persist from run to run on android. In other words, web pages will get unique ones after a restart (like they already get in private browsing mode). Since the deviceId constraint isn't finished yet, this shouldn't matter terribly. Desktop is not affected by this patch (desktop never crashed). [String/UUID change made/needed]: None
Attachment #8603331 -
Attachment is obsolete: false
Attachment #8603331 -
Flags: approval-mozilla-beta?
Attachment #8603331 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8603331 -
Attachment description: Part 1: Turn off persisting of enumerateDevice ids on android (3) r=jesup → Quick-fix: Turn off persisting of enumerateDevice ids on android (3) r=jesup
Updated•9 years ago
|
status-firefox39:
--- → affected
status-firefox40:
--- → affected
Comment 52•9 years ago
|
||
Comment on attachment 8603331 [details] [diff] [review] Quick-fix: Turn off persisting of enumerateDevice ids on android (3) r=jesup This a crash, taking in 39.
Attachment #8603331 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 53•9 years ago
|
||
Previous comment: s/39/40/
Comment 54•9 years ago
|
||
Comment on attachment 8603331 [details] [diff] [review] Quick-fix: Turn off persisting of enumerateDevice ids on android (3) r=jesup Approved for uplift to beta - quick fix for android crash.
Attachment #8603331 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 57•9 years ago
|
||
Verifying as fixed on Firefox 39.0b7 using the steps/sites from description
You need to log in
before you can comment on or make changes to this bug.
Description
•