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

RESOLVED FIXED in Firefox 46

Status

()

P2
critical
Rank:
25
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: bobowen)

Tracking

(Blocks: 1 bug, {crash})

unspecified
mozilla47
Unspecified
Windows NT
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43 wontfix, firefox44 wontfix, firefox45 affected, firefox46 fixed, firefox47 fixed)

Details

(crash signature)

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
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)

Updated

3 years ago
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)

Updated

3 years ago
Rank: 25
Priority: -- → P2

Comment 3

3 years ago
(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)

Comment 4

3 years ago
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)
(Assignee)

Comment 6

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1243674
(Assignee)

Comment 7

3 years ago
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)
(Assignee)

Comment 10

3 years ago
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.
(Assignee)

Comment 11

3 years ago
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
(Assignee)

Comment 12

3 years ago
Created attachment 8716948 [details] [diff] [review]
Part 1: Add new WinUtils function to Resolve moved Users folder
Attachment #8716948 - Flags: review?(jmathies)
(Assignee)

Comment 13

3 years ago
Created attachment 8716949 [details] [diff] [review]
Part 2: Resolve GMP path for moved Users folder
Attachment #8716949 - Flags: review?(cpearce)

Comment 14

3 years ago
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+
(Assignee)

Comment 16

3 years ago
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 19

3 years ago
bugherder
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
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 20

3 years ago
Created attachment 8717843 [details] [diff] [review]
Part 3: Add #ifs to include to fix bustage

I missed the #ifs around the #include.

This was the push in comment 18.
Attachment #8717843 - Flags: review+
(Assignee)

Comment 21

3 years ago
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?
(Assignee)

Comment 22

3 years ago
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?
(Assignee)

Comment 23

3 years ago
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?
status-firefox42: affected → wontfix
status-firefox43: ? → wontfix
status-firefox44: --- → wontfix
status-firefox45: --- → affected
status-firefox46: --- → affected
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+
Depends on: 1273372
(Assignee)

Updated

2 years ago
See Also: → bug 1330930
You need to log in before you can comment on or make changes to this bug.