Closed Bug 1162720 Opened 9 years ago Closed 9 years ago

enumerateDevices crashes on Android (webcamtoy.com crashes)

Categories

(Core :: WebRTC: Audio/Video, defect)

39 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- verified
firefox40 --- fixed
firefox41 --- fixed
fennec 39+ ---

People

(Reporter: jib, Assigned: jib)

References

Details

(Keywords: crash)

Crash Data

Attachments

(4 files, 15 obsolete files)

1.47 KB, patch
jib
: review+
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
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
tracking-fennec: --- → ?
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)
(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).
And M 13 on b2g.
Turn off on B2G as well as the problem shows there.
Attachment #8602988 - Attachment is obsolete: true
Attachment #8603101 - Flags: review?(rjesup)
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)
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+
Attachment #8603102 - Flags: review?(rjesup) → review+
Update comment and #if. Carrying forward r=jesup.
Attachment #8603101 - Attachment is obsolete: true
Attachment #8603331 - Flags: review+
Update commit msg.
Assignee: nobody → jib
Attachment #8603331 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8603332 - Flags: review+
Attachment #8603102 - Attachment is obsolete: true
Attachment #8603331 - Attachment is obsolete: false
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.
Turns out NS_GetSpecialDirectory is main-thread only (see Bug 1163021). Updating code to pass from main.
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
Unbitrot.
Attachment #8603331 - Attachment is obsolete: true
Attachment #8603332 - Attachment is obsolete: true
Attachment #8604437 - Flags: review+
Attachment #8604439 - Attachment description: rename CallbackRunnable::New to NewRunnableFrom → Part 2: rename CallbackRunnable::New to NewRunnableFrom
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)
Attachment #8604439 - Flags: review?(rjesup) → review+
Attachment #8604441 - Flags: review?(rjesup) → review+
Update commit msg.
Attachment #8604437 - Attachment is obsolete: true
Attachment #8605797 - Flags: review+
Update commit msg.
Attachment #8604439 - Attachment is obsolete: true
Attachment #8605798 - Flags: review+
Update commit msg.
Attachment #8604441 - Attachment is obsolete: true
Attachment #8605799 - Flags: review+
Uplift request plz.
tracking-fennec: ? → 39+
Sure, though I was hoping someone would land it on m-i first. ;-)
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
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 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+
Incorporate feedback. Carrying forward r=jesup.
- Added mfbt/* style comments.
- nsAutoTArray<Element, 3>
- whitespace
Attachment #8607208 - Attachment is obsolete: true
Attachment #8608309 - Flags: review+
Minor comment tweak. Carrying forward r=jesup.
Attachment #8608309 - Attachment is obsolete: true
Attachment #8608334 - Flags: review+
Another minor comment tweak, sorry. Carrying forward r=jesup.
Attachment #8608334 - Attachment is obsolete: true
Attachment #8608335 - Flags: review+
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 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 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+
(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
Update commit msg.
Attachment #8609063 - Attachment is obsolete: true
Attachment #8609109 - Flags: review+
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)
I didn't think the base bug was on 39 (though maybe 40) - jib, please check
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
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?
(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)
(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?)
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)
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?
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?
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?
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
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+
Previous comment: s/39/40/
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+
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.