Closed Bug 1236680 Opened 4 years ago Closed 4 years ago

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

Categories

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

Unspecified
Windows NT
defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- affected
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: cpeterson, Assigned: bobowen)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

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
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.
Component: Audio/Video: Playback → Audio/Video: GMP
Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
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
Flags: needinfo?(hankpeng)
Rank: 25
Priority: -- → P2
(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.
Flags: needinfo?(hankpeng)
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?
Flags: needinfo?(rjesup)
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
Flags: needinfo?(rjesup)
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.
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
Blocks: 1243674
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?
Flags: needinfo?(cpearce)
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.
A build with these patches was able to work in my VM with a C:\Users directory which was a junction to D:\Users.
Flags: needinfo?(cpearce)
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.
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 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
Attachment #8716948 - Flags: review?(jmathies) → review+
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.
Attachment #8716949 - Flags: review?(cpearce) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/058a07de7cb6
https://hg.mozilla.org/mozilla-central/rev/5bf10cf78faf
https://hg.mozilla.org/mozilla-central/rev/e3509c59fb6a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
I missed the #ifs around the #include.

This was the push in comment 18.
Attachment #8717843 - Flags: review+
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
Attachment #8716948 - Flags: approval-mozilla-beta?
Attachment #8716948 - Flags: approval-mozilla-aurora?
Comment on attachment 8716949 [details] [diff] [review]
Part 2: Resolve GMP path for moved Users folder

See comment 21.
Attachment #8716949 - Flags: approval-mozilla-beta?
Attachment #8716949 - Flags: approval-mozilla-aurora?
Comment on attachment 8717843 [details] [diff] [review]
Part 3: Add #ifs to include to fix bustage

See comment 21.
Attachment #8717843 - Flags: approval-mozilla-beta?
Attachment #8717843 - Flags: approval-mozilla-aurora?
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
Attachment #8716948 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8716949 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8717843 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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.
Attachment #8716948 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8716949 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8717843 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1330930
You need to log in before you can comment on or make changes to this bug.