Closed Bug 1043531 Opened 5 years ago Closed 5 years ago

OpenH264 crash reporting notification not shown

Categories

(Firefox :: General, defect)

defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.1
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 7 obsolete files)

1.18 KB, patch
smaug
: review+
gfritzsche
: checkin+
Details | Diff | Splinter Review
4.25 KB, patch
jesup
: review+
gfritzsche
: checkin+
Details | Diff | Splinter Review
13.60 KB, patch
cpearce
: review+
gfritzsche
: checkin+
Details | Diff | Splinter Review
1.59 KB, patch
ttaubert
: review+
gfritzsche
: checkin+
Details | Diff | Splinter Review
9.09 KB, patch
gfritzsche
: review+
gfritzsche
: checkin+
Details | Diff | Splinter Review
With bug 1041226, bug 1041525, bug 1041232 applied and hence crashreporting basically working again, i still don't see any crash notification.
Flags: firefox-backlog+
Component: WebRTC → General
Product: Core → Firefox
Version: unspecified → Trunk
QA Whiteboard: [qa+]
Points: 5 → 8
Attachment #8461733 - Flags: review?(rjesup)
With the above patches we get at least to here:
http://hg.mozilla.org/mozilla-central/annotate/06ac51c2b8a8/dom/media/PeerConnection.js#l118

Out of the door now, can pick up again tomorrow.
Test-locations/-files suggested?
Added to Iteration 34.1
Attachment #8461733 - Flags: review?(rjesup) → review+
Patch 2 only (leave-open):
https://hg.mozilla.org/integration/mozilla-inbound/rev/edeed66fba28
Whiteboard: [openh264-uplift] → [openh264-uplift][leave-open]
Comment on attachment 8461723 [details] [diff] [review]
Part 1: Fix PeerConnection WebIDL

Smaug, looks like you're a candidate for review?
The implementation is actually using uint64_t for plugin ids, this just changes the interface to match.
Attachment #8461723 - Flags: review?(bugs)
Attachment #8461723 - Flags: review?(bugs) → review+
QA Contact: anthony.s.hughes
This appears to work for all cases: multiple calls (process sticks around); plugin crash (shut down GMPParent and clone it to avoid the same Close()/Open() IPC bug).  Note: The JS code to send the event doesn't work; I talked with bz and smaug about how to fix it.  Need to move it to C++/PeerConnectionImpl
Attached patch WIP Event delivery (obsolete) — Splinter Review
Per a #content discussion it's hard to do the event delivery from JS, so we'll do it in CPP like nsObjectLoadingContent.cpp.
Ideally we'll share code with nsObjectLoadingContent - once this actually works.

I still don't get any event in browser-plugins.js though (see dump() statement for expected target) with adopting that code.
I tried with both passing the window in (this patch version) and using PeerConnectionImpl::mWindow.

Tracing the event dispatching code path is a little weird, but we get null for piTarget here:
http://hg.mozilla.org/mozilla-central/annotate/de8c0f0e74a2/dom/events/EventDispatcher.cpp#l445
... so we seem to not have a proper chrome target setup here?

smaug, bz, any ideas?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
cleanup and delete/re-add the plugin whenever it goes back to idle (until that other bug is fixed).  Avoids a process leaking-until-shutdown.
Attachment #8462980 - Attachment is obsolete: true
Attachment #8463083 - Flags: review?(cpearce)
(In reply to Georg Fritzsche [:gfritzsche] from comment #9) 
> Tracing the event dispatching code path is a little weird, but we get null
> for piTarget here:
> http://hg.mozilla.org/mozilla-central/annotate/de8c0f0e74a2/dom/events/
> EventDispatcher.cpp#l445
> ... so we seem to not have a proper chrome target setup here?
That is really odd. Are you dispatching the event when the window has been already closed?
Flags: needinfo?(bugs)
jesup can't reproduce the issue gfritzsche got.
Dump is printed an all http://logs.glob.uno/?c=content#c228625
Flags: needinfo?(bzbarsky)
I still can't get the events delivered even on newest & clobbered inbound.
It's working for jesup on Linux while i'm trying on OS X.

I can go for the crashreporter popup fix on Linux, but we still need to figure out whether i'm experiencing something odd or if there's something going wrong on Mac.
Works for me on inbound on mac with those patches
Attachment #8463083 - Attachment is obsolete: true
Attachment #8463083 - Flags: review?(cpearce)
Comment on attachment 8463244 [details] [diff] [review]
Safely delete GMP processes when unused and handle plugin crashes

This is the full patch; interdiffs against the previous one are also uploaded.

Clone code stolen from bug 1039886
Attachment #8463244 - Flags: review?(cpearce)
I should also note I tested the updated patchqueue fairly well (all the basic cases: repeated calls, disable/enable via AddOns, Disable in-call, killing plugins externally (generated crash event).
Comment on attachment 8463244 [details] [diff] [review]
Safely delete GMP processes when unused and handle plugin crashes

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

::: content/media/gmp/GMPParent.cpp
@@ +208,5 @@
>  {
>    LOGD(("%s::%s: %p", __CLASS__, __FUNCTION__, this));
> +  // Don't Close() twice!
> +  // Probably remove when bug 1043671 is resolved
> +  if (mState == GMPStateClosing) {

As far as I can tell, DeleteProcess() is only called directly after setting mState=GMPStateClosing? So maybe you should assert that mState==GMPStateClosing here? Do you actually need the "if" statement to check if you're in GMPStateClosing state.

::: content/media/gmp/GMPService.cpp
@@ +567,5 @@
> +{
> +  MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> +  LOGD(("%s::%s: %p", __CLASS__, __FUNCTION__, (void*) aOld));
> +
> +  nsRefPtr<GMPParent> gmp = ClonePlugin(aOld);

Since there's only one caller of ClonePlugin, I'd just have it return the plugin without inserting it into the list, so you don't need to drop the lock between inserting and deleting, or justify why it should be safe to do so.
Attachment #8463244 - Flags: review?(cpearce) → review+
Tim, simple fix that maybe you can sign off?
Attachment #8463383 - Flags: review?(ttaubert)
Attachment #8463383 - Flags: review?(ttaubert) → review+
Re the shared location for the CustomEvent code:
<•smaug> gfritzsche: nsContentUtils
<•smaug> or DOMWindowUtils
<•smaug> DOMWindowUtils would be accessible from JS too
<bz> Or just a chromeonly static method on CustomEvent... ;)
<•smaug> oh right, CustomEvent isn't generated anymore
<•smaug> it used to be when it didn't use webidl codegen but the xpidl codegen
Per IRC:
<•smaug> gfritzsche: one option is to just use event codegen to generate a new type of event, but expose it ChromeOnly
<•smaug> that is probably the sanest approach. No need to use propertybag or anything, just sane property names in the event

That sounds best, but we're fine with doing this as a follow-up to keep the risk low for the required uplifts here.
Needs follow-up bug.
Attachment #8463426 - Flags: review?(rjesup)
Attachment #8463426 - Flags: review?(rjesup) → review+
Attachment #8461733 - Flags: checkin+
Attachment #8463014 - Attachment is obsolete: true
Attachment #8463244 - Flags: checkin+
Attachment #8463426 - Flags: review?(bugs)
Blocks: 1045100
Comment on attachment 8463426 [details] [diff] [review]
Move PluginCrashed event creation to C++

>+            pc._pc.pluginCrash(pc._win, pluginID, name, crashReportID);
You shouldn't need the first param.
So, please re-test ;)



>-PeerConnectionImpl::PluginCrash(uint64_t aPluginID)
>+PeerConnectionImpl::PluginCrash(uint64_t aPluginID,
>+                                const nsAString& pluginName,
>+                                const nsAString& pluginDumpID)
In C++ argument names should be in form aFoo.
So, aPluginName, aPluginDumpID. Same also in .h


The bag is missing pluginFilename and browserDumpID but I assume that is fine here.
Attachment #8463426 - Flags: review?(bugs) → review+
Addressed above comments, retested properly.
Attachment #8463426 - Attachment is obsolete: true
Attachment #8463528 - Flags: review+
Keywords: checkin-needed
Attachment #8463241 - Attachment is obsolete: true
Comment on attachment 8463244 [details] [diff] [review]
Safely delete GMP processes when unused and handle plugin crashes

This is for all patches on this bug other than "Patch 2" which landed in 33.
Note that I have versions of these in a patch queue merged to Aurora along with the other OpenH264 a+'d patches (so we can move Aurora directly forward to a known-stable-state from m-c).  This set (and bug 1044550 and bug 1041226) are the last ones needed to land in Aurora.  I've been doing local Aurora+patchset tests locally.

Note: because of tree closures, some of these have been waiting to merge all day; the last two have been waiting to land.

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Leaks and errors if an GMP plugin fails; also fixes immediate crash on Mac Aurora in OpenH264 calls.

[Describe test coverage new/current, TBPL]: GMP fake plugin tests are on m-c and will be merged ASAP (bug 1041347, already a+ for aurora).  They do catch problems here; without these patches, there was a leak on central that was missed because a separate bug had disabled all leak checks.

[Risks and why]: This will bring Aurora to a par with m-c for stability and will enable plugin crash reporting.  m-c and Aurora with these patches are such that I can't force them to fail or leak (without these you have problems after around 4 calls).

[String/UUID change made/needed]: none
Attachment #8463244 - Flags: approval-mozilla-aurora?
Attachment #8461723 - Flags: approval-mozilla-aurora?
Attachment #8463383 - Flags: approval-mozilla-aurora?
Attachment #8463528 - Flags: approval-mozilla-aurora?
With tweaked Win32 compat changes to #includes, for checkin
Comment on attachment 8463839 [details] [diff] [review]
Move PluginCrashed event generation to C++.

Carry forward r=jesup,smaug.
Attachment #8463839 - Flags: review+
Attachment #8463839 - Flags: approval-mozilla-aurora?
Attachment #8463528 - Attachment is obsolete: true
Attachment #8463528 - Flags: approval-mozilla-aurora?
Attachment #8463839 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8463383 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8463244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8461723 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Removed left-over testing chunk.
Already has approval-mozilla-aurora+ above.
Attachment #8463839 - Attachment is obsolete: true
Attachment #8463843 - Flags: review+
Depends on: 1045500
Keywords: checkin-needed
Attachment #8461723 - Flags: checkin+
Attachment #8463383 - Flags: checkin+
Attachment #8463843 - Flags: checkin+
Whiteboard: [openh264-uplift][leave-open] → [openh264-uplift]
https://hg.mozilla.org/mozilla-central/rev/e5506d8f41a6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment on attachment 8461733 [details] [diff] [review]
Part 2: Cache plugin id

Approval Request Comment:
This didn't actually get into 33, so requesting approval per below comment as well.


(In reply to Randell Jesup [:jesup] from comment #29)
> Comment on attachment 8463244 [details] [diff] [review]
> Safely delete GMP processes when unused and handle plugin crashes
> 
> This is for all patches on this bug other than "Patch 2" which landed in 33.
> Note that I have versions of these in a patch queue merged to Aurora along
> with the other OpenH264 a+'d patches (so we can move Aurora directly forward
> to a known-stable-state from m-c).  This set (and bug 1044550 and bug
> 1041226) are the last ones needed to land in Aurora.  I've been doing local
> Aurora+patchset tests locally.
> 
> Note: because of tree closures, some of these have been waiting to merge all
> day; the last two have been waiting to land.
> 
> Approval Request Comment
> [Feature/regressing bug #]: N/A
> 
> [User impact if declined]: Leaks and errors if an GMP plugin fails; also
> fixes immediate crash on Mac Aurora in OpenH264 calls.
> 
> [Describe test coverage new/current, TBPL]: GMP fake plugin tests are on m-c
> and will be merged ASAP (bug 1041347, already a+ for aurora).  They do catch
> problems here; without these patches, there was a leak on central that was
> missed because a separate bug had disabled all leak checks.
> 
> [Risks and why]: This will bring Aurora to a par with m-c for stability and
> will enable plugin crash reporting.  m-c and Aurora with these patches are
> such that I can't force them to fail or leak (without these you have
> problems after around 4 calls).
> 
> [String/UUID change made/needed]: none
Attachment #8461733 - Flags: approval-mozilla-aurora?
Attachment #8461733 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Georg, Florin has asked for details on how to verify this bug.  Can you provide him with that.
Flags: needinfo?(georg.fritzsche)
* wait for OpenH264 plugin being installed (tools -> addons -> plugins)
* go to http://mozilla.github.io/webrtc-landing/pc_test.html
* start test with "require h264 video" checked
* there should be a plugin-container process running now with the OpenH264 GMP
* kill the process with e.g. ABORT signal (|kill -ABRT <pid>|)
* a notification bar should come up: http://imgur.com/IRPJSl4
(note the name is still cut off in the pic)
Flags: needinfo?(georg.fritzsche)
Verified as fixed with Aurora 33.0a2 (Build ID: 20140731004002) and Nightly 34.0a1 (Build ID: 20140731030206) builds on Mac OS X 10.9.4 and Ubuntu 13.04 64bit - OpenH264 crash reporting notification is properly shown.
On Windows 7 x64, the "Submit a crash report" button is missing. Logged bug 1047374.
Marking verified fixed based on comment 40.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.