Closed
Bug 1236680
Opened 9 years ago
Closed 8 years ago
Result "not reached" crash in mozilla::gmp::GMPChild::ProcessingError()
Categories
(Core :: Audio/Video: GMP, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: cpeterson, Assigned: bobowen)
References
(Blocks 1 open bug)
Details
(Keywords: crash)
Crash Data
Attachments
(3 files)
8.24 KB,
patch
|
jimm
:
review+
lizzard
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
cpearce
:
review+
lizzard
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
984 bytes,
patch
|
bobowen
:
review+
lizzard
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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•9 years ago
|
Flags: needinfo?(rjesup)
Comment 2•9 years ago
|
||
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•9 years ago
|
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)
Comment 5•8 years ago
|
||
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•8 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 | ||
Comment 7•8 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)
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
Attachment #8716948 -
Flags: review?(jmathies)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8716949 -
Flags: review?(cpearce)
Comment 14•8 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 15•8 years ago
|
||
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•8 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 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/058a07de7cb6 https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf10cf78faf
Comment 19•8 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
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 20•8 years ago
|
||
I missed the #ifs around the #include. This was the push in comment 18.
Attachment #8717843 -
Flags: review+
Assignee | ||
Comment 21•8 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•8 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•8 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?
Updated•8 years ago
|
Comment 24•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8716949 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•8 years ago
|
Attachment #8717843 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 25•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8716949 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8717843 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 26•8 years ago
|
||
bugherder uplift |
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
You need to log in
before you can comment on or make changes to this bug.
Description
•