1236680 - Result "not reached" crash in mozilla::gmp::GMPChild::ProcessingError()

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect, P2)

Description

[Chris Peterson [:cpeterson]]

This bug was filed from the Socorro interface and is 
report bp-7485467a-ad2b-49a4-a013-30f5a2151229.
=============================================================

GMPChild::ProcessingError() is called with an unexpected Result code. We had a similar ProcessingError() crash in bug 1164245.

http://hg.mozilla.org/releases/mozilla-release/annotate/e2f9a0ed50bc/dom/media/gmp/GMPChild.cpp#l532


Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::gmp::GMPChild::ProcessingError(mozilla::ipc::HasResultCodes::Result, char const*) 	dom/media/gmp/GMPChild.cpp
1 	xul.dll 	mozilla::ipc::MessageChannel::MaybeHandleError(mozilla::ipc::HasResultCodes::Result, IPC::Message const&, char const*) 	ipc/glue/MessageChannel.cpp
2 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
3 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp
4 	xul.dll 	RunnableMethod<SoftwareDisplay, void ( SoftwareDisplay::*)(void), Tuple0>::Run() 	ipc/chromium/src/base/task.h
5 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
6 	xul.dll 	base::MessagePumpDefault::Run(base::MessagePump::Delegate*) 	ipc/chromium/src/base/message_pump_default.cc
7 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
8 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
9 	xul.dll 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp
10 	plugin-container.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
11 	plugin-container.exe 	__tmainCRTStartup 	f:/dd/vctools/crt/crtw32/startup/crt0.c:255
12 	kernel32.dll 	BaseThreadInitThunk 	
13 	ntdll.dll 	RtlUserThreadStart 	
14 	kernel32.dll 	BasepReportFault 	
15 	kernel32.dll 	BasepReportFault

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

This crash is happening with OpenH264 version 1.5.1. So moving into "A/V: GMP" component, as I assume it's not Playback related.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Not sure why I cleared the NI; may have been accidental.  Aha, I probably added hank but didn't NI him.  Since the changes from 1.5.1 to 1.5.3 likely didn't affect this, it probably is still valid  If so, it might need to move to plugins/openh264

Comment 3

[Hank Peng]

(In reply to Randell Jesup [:jesup] from comment #2)
> Not sure why I cleared the NI; may have been accidental.  Aha, I probably
> added hank but didn't NI him.  Since the changes from 1.5.1 to 1.5.3 likely
> didn't affect this, it probably is still valid  If so, it might need to move
> to plugins/openh264

I am looking into it now. Thanks.

Comment 4

[Hank Peng]

Hi Randell, I didn't find any clue in openh264 plugin that may lead to this bug after doing some investigation. I still have no idea about the root cause. 
I searched the crash signature in mozilla crash reports. The result showed this crash occurs with other plugins besides openh264. In the past one month, this crash happens 119 times. All crashes are on Windows.  Openh264 v1.5.1 contributes 4 times, openh264 v1.5.2 1 time, clearkey v1 9 times, and all the left are related to eme-adobe. Please have a look at the crash report:

https://crash-stats.mozilla.com/signature/?date=%3E%3D2015-12-15T00%3A00%3A00+00%3A00&date=%3C2016-01-12T00%3A00%3A00+00%3A00&signature=mozilla%3A%3Agmp%3A%3AGMPChild%3A%3AProcessingError&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=plugin_filename&_columns=plugin_version&page=1#reports

Do you have any comments about how to look into it next?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

I was able to make contact with a user who reported seeing this on reddit:

https://www.reddit.com/r/firefox/comments/41s0x7/help_netflix_bug_i_cant_solve/

After talking to the user I was able to reproduce this bug! :D

He'd copied his c:\Users folder to another drive, and created a junction/hard-symlink in C:\Users pointing to the new location. This basically means the Windows Users dir appears to be on drive C:, but actually is stored on another drive, and most Windows programs don't notice. Most programs, except us of course.

I followed the instructions I found on lifehacker to do the same in my Win7 VM and I can repro the bug!
http://lifehacker.com/5467758/move-the-users-directory-in-windows-7

Comment 6

[Bob Owen (:bobowen)]

From my email ...

I think the best approach is to try and detect this particular workaround of moving the Users folder and only resolve in that one situation.
That way we keep the attack surface to a minimum.

From my testing it seems standard users can create folders and junction points on the drive root.
However to move the Users folder is going to need Administrator access and it will require some sort of system recovery prompt anyway.

Comment 7

[Bob Owen (:bobowen)]

Try push that failed to take account that Users doesn't exist pre-Vista:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f0cef2fb73

Try push with that fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4714c1562c05

Chris - do you want to see if that build fixes the issue on your VM and also for the guy who originally had the problem?
Also, is it worth me running any other test suites or are all the GMP/EME tests in M-2?

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

I had to head home before I was able to test this in my VM at the office, and my VPN connection no longer works. Will try tomorrow.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

A build with these patches was able to work in my VM with a C:\Users directory which was a junction to D:\Users.

Comment 10

[Bob Owen (:bobowen)]

Damn, I've just realised that these patches will only work if they have called the new folder Users and put it in the root of a different drive.

I think I'm going to have to go through the pain of properly resolving the reparse point, which I was trying to avoid.

Comment 11

[Bob Owen (:bobowen)]

Here's a try push with proper resolution of the junction point, in case the name is different or it isn't in the root of the drive:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d09540d53964

Comment 12

[Bob Owen (:bobowen)]

Attachment: Part 1: Add new WinUtils function to Resolve moved Users folder

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 2: Resolve GMP path for moved Users folder

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 8716948 [details] [diff] [review]
Part 1: Add new WinUtils function to Resolve moved Users folder

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

::: widget/windows/WinUtils.cpp
@@ +1832,5 @@
> +    return true;
> +  }
> +
> +  wchar_t* usersPath;
> +  if (SHGetKnownFolderPath(FOLDERID_UserProfiles, 0, nullptr, &usersPath) != S_OK) {

nit - FAILED() test vs. direct compare to S_OK. Lets also add the 'WinUtils' reference to this call so we know where the call is going.

@@ +1851,5 @@
> +  DWORD attributes = ::GetFileAttributesW(usersPath);
> +  if (attributes == INVALID_FILE_ATTRIBUTES) {
> +    return false;
> +  }
> +  if (!(attributes & FILE_ATTRIBUTE_REPARSE_POINT)) {

nit - comment this check

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8716949 [details] [diff] [review]
Part 2: Resolve GMP path for moved Users folder

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

I think the code in GMPChild::SetMacSandboxInfo(), in the call to GetAppPaths() resolves soft links (to anywhere, not just in the home directory) as well.

::: dom/media/gmp/GMPProcessParent.cpp
@@ +59,4 @@
>  
>  #if defined(XP_WIN) && defined(MOZ_SANDBOX)
>    std::wstring wGMPPath = UTF8ToWide(mGMPPath.c_str());
> +  if (!widget::WinUtils::ResolveMovedUsersFolder(wGMPPath)) {

Some comments including the bug number here would be good, as it's not obvious as to the motivation why we need to do this.

Comment 16

[Bob Owen (:bobowen)]

Thanks both, changes made locally.

(In reply to Jim Mathies [:jimm] from comment #14)

> > +  if (SHGetKnownFolderPath(FOLDERID_UserProfiles, 0, nullptr, &usersPath) != S_OK) {
> 
> nit - FAILED() test vs. direct compare to S_OK. Lets also add the 'WinUtils'
> reference to this call so we know where the call is going.

Done, thanks.
I meant to put WinUtils in there, but forgot.
 
> @@ +1851,5 @@
> > +  DWORD attributes = ::GetFileAttributesW(usersPath);
> > +  if (attributes == INVALID_FILE_ATTRIBUTES) {
> > +    return false;
> > +  }
> > +  if (!(attributes & FILE_ATTRIBUTE_REPARSE_POINT)) {
> 
> nit - comment this check

Moved the comment down from above and made it more meaningful.


(In reply to Chris Pearce (:cpearce) from comment #15)

> I think the code in GMPChild::SetMacSandboxInfo(), in the call to
> GetAppPaths() resolves soft links (to anywhere, not just in the home
> directory) as well.

I wonder if we need to look into that.

> >  #if defined(XP_WIN) && defined(MOZ_SANDBOX)
> >    std::wstring wGMPPath = UTF8ToWide(mGMPPath.c_str());
> > +  if (!widget::WinUtils::ResolveMovedUsersFolder(wGMPPath)) {
> 
> Some comments including the bug number here would be good, as it's not
> obvious as to the motivation why we need to do this.

Added.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/058a07de7cb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf10cf78faf

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3509c59fb6a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/058a07de7cb6
https://hg.mozilla.org/mozilla-central/rev/5bf10cf78faf
https://hg.mozilla.org/mozilla-central/rev/e3509c59fb6a

Comment 20

[Bob Owen (:bobowen)]

Attachment: Part 3: Add #ifs to include to fix bustage

I missed the #ifs around the #include.

This was the push in comment 18.

Comment 21

[Bob Owen (:bobowen)]

Comment on attachment 8716948 [details] [diff] [review]
Part 1: Add new WinUtils function to Resolve moved Users folder

Nightly finally got built!

Approval Request Comment
[Feature/regressing bug #]:
Bug has been there since GMP shipped.

[User impact if declined]:
Users who have moved their Users folder using a junction point will still not be able to use GMP (openh264 or EME).

[Describe test coverage new/current, TreeHerder]:
GMP has test coverage in tree.
The particular circumstance of a moved Users folder would be difficult to add a test for, but this has been tested manually with the latest Nightly.

[Risks and why]:
Low to medium - all GMP process start ups go through a small part of the new code, but that part is pretty simple. If the Users folder has been moved it's a bit more complicated, but would only affect those cases which don't work now.

[String/UUID change made/needed]:
None

Comment 22

[Bob Owen (:bobowen)]

Comment on attachment 8716949 [details] [diff] [review]
Part 2: Resolve GMP path for moved Users folder

See comment 21.

Comment 23

[Bob Owen (:bobowen)]

Comment on attachment 8717843 [details] [diff] [review]
Part 3: Add #ifs to include to fix bustage

See comment 21.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8716948 [details] [diff] [review]
Part 1: Add new WinUtils function to Resolve moved Users folder

Risky, only a few crashes, not taking it in 45

Comment 25

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

Comment on attachment 8716948 [details] [diff] [review]
Part 1: Add new WinUtils function to Resolve moved Users folder

Crash fix for some Windows users, ok in Nightly, let's give it a try in aurora.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/d46a689e9e67
https://hg.mozilla.org/releases/mozilla-aurora/rev/8525cbc2a633
https://hg.mozilla.org/releases/mozilla-aurora/rev/9892d1ef1d85