Closed Bug 1045100 Opened 10 years ago Closed 10 years ago

Unify custom event "PluginCrashed" in shared location

Categories

(Core :: General, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
36.3

People

(Reporter: gfritzsche, Assigned: cojo, Mentored)

References

Details

(Whiteboard: [lang=c++][lang=js])

Attachments

(2 files, 16 obsolete files)

9.10 KB, patch
cojo
: review+
Details | Diff | Splinter Review
15.32 KB, patch
mconley
: review+
Details | Diff | Splinter Review
Per bug 1043531, we now have two code to create the custom event "PluginCrashed" in two places:
* PeerConnectionImpl::PluginCrash()
* nsObjectLoadingContent.cpps nsPluginCrashedEvent::Run()

We should unify these. The best sounding option suggested by smaug:
> one option is to just use event codegen to generate a new type of event, but expose it ChromeOnly
> that is probably the sanest approach. No need to use propertybag or anything, just sane property names in the event

Context:
bug 1043531, comment 9
bug 1043531, comment 23
bug 1043531, comment 24
Flags: firefox-backlog+
smaug, can you point to resources & existing examples on the event codegen?
Given that this would probably be a well-scoped mentored bug.
Flags: needinfo?(bugs)
So you'd create an interface similar to http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ProgressEvent.webidl?force=1 (with whatever properties you need on it). Properties in the dictionary
should map to the attributes in the interface, and dictionary should have default values.
"" should be fine for DOMString property.
Since we wouldn't want to expose the interface to web content, it should be marked with 
[ChromeOnly]


Then add the interface to http://mxr.mozilla.org/mozilla-central/source/dom/webidl/moz.build?rev=326c91338df0#631
and in C++ then initialize and dispatch the event something like 
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp?rev=3baea94810b1#1486, though you may want to set that chromeonly flag.
Flags: needinfo?(bugs)
The locations we want to change to use that event are around here:
http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#l1680
http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/content/base/src/nsObjectLoadingContent.cpp#l322

As we will end up with nice actual properties we need to change the frontend code and the tests that use the event.

The event ends up being used in the front-end here:
http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/browser/base/content/browser-plugins.js#l1154

The tests we need to check primarily are:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_CTP_crashreporting.js
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_busy_hang.xul
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_crash_notify.xul
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_crash_notify_no_report.xul
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_crash_submit.xul
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_hang_submit.xul
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_hangui.xul
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_hangui.xul


Some of them may need adjustment similar to the frontend code, at least one needs to change how it fakes the "PluginCrashed" event.
Mentor: georg.fritzsche
Points: 3 → 5
Whiteboard: [lang=c++][lang=js]
Not sure if this bug is still active, but I figured a good place to start would be to create a PluginCrashedEvent interface with the properties found at http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#l1708 and http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/content/base/src/nsObjectLoadingContent.cpp#l348.

Looking at some of the front-end code http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/browser/base/content/browser-plugins.js#l1168 it seems like two properties "browserDumpID" and "gmpPlugin" can be null, so I made those nullable properties.

It seems like, from the front-end code, that two more properties could be added: "doPrompt" and "submitReports" http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/browser/base/content/browser-plugins.js#l1161

This patch was just a first step. Not sure if I'm headed in the right direction or not (or if this bug makes sense to work on), so I didn't want to go too far.
Hi Cojo, sorry for the delay here - this slipped past me somehow and i'll take a look tomorrow.
No worries. I figured I'd only do some preliminary work (starting to create the interface) to see if this bug was still relevant/in the works. Since it seems like it's still active I'll start following up where I left off.
Comment on attachment 8487271 [details] [diff] [review]
1045100-unify-plugin-crashed.diff

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

Smaug, do you mind taking a quick look?

::: dom/webidl/PluginCrashedEvent.webidl
@@ +7,5 @@
> +[Constructor(DOMString type, optional PluginCrashedEventInit eventInitDict), ChromeOnly]
> +interface PluginCrashedEvent : Event
> +{
> +  readonly attribute DOMString pluginDumpID;
> +  readonly attribute DOMString browserDumpID;

From other examples in the tree, this should probably be |DOMString?|.
Attachment #8487271 - Flags: feedback?(bugs)
Great, thanks - yes, this is still active.
Note that bug 899347 (currently landing, only bounced for some test), changes browser-plugins.js - it splits it up between that file and PluginContent.jsm. The contents didn't really change though.

(In reply to Connor [:cojo] from comment #4)
> Not sure if this bug is still active, but I figured a good place to start
> would be to create a PluginCrashedEvent interface with the properties found
> at
> http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/media/webrtc/
> signaling/src/peerconnection/PeerConnectionImpl.cpp#l1708 and
> http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/content/base/src/
> nsObjectLoadingContent.cpp#l348.

This looks good from the contents.

> Looking at some of the front-end code
> http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/browser/base/
> content/browser-plugins.js#l1168 it seems like two properties
> "browserDumpID" and "gmpPlugin" can be null, so I made those nullable
> properties.

Right, this sounds good.

> It seems like, from the front-end code, that two more properties could be
> added: "doPrompt" and "submitReports"
> http://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/browser/base/
> content/browser-plugins.js#l1161

Hm, for doPrompt & shouldSubmit i don't see immediately what we should do. I'd defer changing them to a separate bug or patch on this bug.
(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> ::: dom/webidl/PluginCrashedEvent.webidl
> @@ +7,5 @@
> > +[Constructor(DOMString type, optional PluginCrashedEventInit eventInitDict), ChromeOnly]
> > +interface PluginCrashedEvent : Event
> > +{
> > +  readonly attribute DOMString pluginDumpID;
> > +  readonly attribute DOMString browserDumpID;
> 
> From other examples in the tree, this should probably be |DOMString?|.
Well, the types in the EventInit and in the Event should be the same.
So either both are nullables ('?'), or neither is.
Attachment #8487271 - Flags: feedback?(bugs)
Yep, that makes sense. The gmpPlugin property can also be nullable, so I'll make both gmpPlugin and browserDumpID nullable in the PluginCrashedEvent interface. I'll work on adding the interface to moz.build and initializing/dispatching the event next.
I've updated the interface and did a first pass at the implementation. I haven't yet touched the front end piece, will do that if the first pass work looks good. 

Before I do, I wanted to check if, in the patch I attached, the GlobalObject being passed to the PluginCrashedEvent::Constructor is correct.

I'm not sure if `doc` and `mContent` are the correct in the following snippets. Should the GlobalObject be `doc` for both of them? Or, is `doc` even the correct GlobalObject to pass? I've just been referencing [1], so not quite sure if I've got it right. 

Snippets:

> nsRefPtr<PluginCrashedEvent> event =
>    PluginCrashedEvent::Constructor(doc, NS_LITERAL_STRING("PluginCrashed"), init);

>  nsRefPtr<PluginCrashedEvent> event =
>    PluginCrashedEvent::Constructor(mContent, NS_LITERAL_STRING("PluginCrashed"), init);

[1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#C.2B.2B_reflections_of_WebIDL_constructors
Attachment #8487271 - Attachment is obsolete: true
Attachment #8491288 - Flags: review?(georg.fritzsche)
Assignee: nobody → cojojennings
Status: NEW → ASSIGNED
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
(In reply to Connor [:cojo] from comment #11)
> Before I do, I wanted to check if, in the patch I attached, the GlobalObject
> being passed to the PluginCrashedEvent::Constructor is correct.
> 
> I'm not sure if `doc` and `mContent` are the correct in the following
> snippets. Should the GlobalObject be `doc` for both of them? Or, is `doc`
> even the correct GlobalObject to pass? I've just been referencing [1], so
> not quite sure if I've got it right. 

From looking at other examples in the tree, probably the event target - so mWindow & mContent respectively.
But thats a better question for smaug or - if things are already working that way - just for the review pass by him.

Note that you have trailing whitespace on new & empty lines, its easy to see - marked red - when you follow the review link for the patch :)
Comment on attachment 8491288 [details] [diff] [review]
1045100-unify-plugin-crashed-2.diff

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

This covers what we need and already looks much cleaner, nice.
Have you tried building it already?
Attachment #8491288 - Flags: review?(georg.fritzsche) → feedback+
Yep, it builds. I did try building with mWindow before attaching the most recent patch and that ended up failing, which is why I was curious (and why I ended up using `doc` and `mContent`, respectively). It seems that [1] would imply mWindow should work, but could just be reading that incorrectly.

I'll work on implementing the front end piece and see if there are issues.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#C.2B.2B_reflections_of_WebIDL_constructors
One question about using this event on the front end. Currently it looks like a custom event is created here [1] and the various properties of that event are filled in a `propBag`. Should this portion of the code be modified to use this new interface? (Instead of creating a custom event, do you instead create a "PluginCrashedEvent" event from the front end and populate what was previously the `propBag` with a js object that maps to the "PluginCrashedEventInit" dict?

Not sure if I'm looking in the right place or heading in the right direction. Just curious after doing some poking around.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js#44
(In reply to Connor [:cojo] from comment #15)
> One question about using this event on the front end. Currently it looks
> like a custom event is created here [1] and the various properties of that
> event are filled in a `propBag`. Should this portion of the code be modified
> to use this new interface?

Yes, we should use the new event there as well.
(In reply to Connor [:cojo] from comment #14)
> Yep, it builds. I did try building with mWindow before attaching the most
> recent patch and that ended up failing, which is why I was curious (and why
> I ended up using `doc` and `mContent`, respectively). It seems that [1]
> would imply mWindow should work, but could just be reading that incorrectly.

smaug, should we use mWindow here? This is in attachment 8491288 [details] [diff] [review], PeerConnectionImpl.cpp.
Flags: needinfo?(bugs)
What does "ended up failing" mean here? Ended up not compiling?
You may need to case mWindow to nsGlobalWindow which inherits EventTarget.
static_cast<nsGlobalWindow*>(mWindow.get());
Flags: needinfo?(bugs)
Yep, sorry, meant it did not compile.
I worked on getting the front end code and tests working. I was able to run mochitest-browser/mochitest-chrome on all the tests except [1]. (The output when attempting to run [1] contained the following: "TEST-SKIPPED | dom/plugins/test/mochitest/test_hangui.xul | skip-if: (buildapp == "mulet") || ((!crashreporter) || (os != "win"))"). All the tests (besides [1]) passed successfully.

I ended up removing a lot of the following checks (this one found at [2], provided as an example):
>     if (!(aEvent instanceof Ci.nsIDOMCustomEvent))
>      return;

Not sure how to check for an "instanceof PluginCrashedEvent" in the tests or in the example above, but would like to look into it.

Lastly, there seems to be a segment of code in [3] that could be replaced by the PluginCrashedEvent, but I wasn't sure if 1) the segment of code could be replaced by PluginCrashedEvent and 2) how to replace it if it could. So I did not touch [3] and [4] ([4] is an example of where it seems to be utilized on the front end).

[1] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_hangui.xul
[2] http://dxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#802
[3] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#3505
[4] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_crash_notify.xul#30
Attachment #8491288 - Attachment is obsolete: true
(In reply to Connor [:cojo] from comment #20)
> I worked on getting the front end code and tests working. I was able to run
> mochitest-browser/mochitest-chrome on all the tests except [1]. (The output
> when attempting to run [1] contained the following: "TEST-SKIPPED |
> dom/plugins/test/mochitest/test_hangui.xul | skip-if: (buildapp == "mulet")
> || ((!crashreporter) || (os != "win"))"). All the tests (besides [1]) passed
> successfully.

Ah, you need to enable the crashreporter in your mozconfig for this:
> ac_add_options --enable-crashreporter

> I ended up removing a lot of the following checks (this one found at [2],
> provided as an example):
> >     if (!(aEvent instanceof Ci.nsIDOMCustomEvent))
> >      return;
> 
> Not sure how to check for an "instanceof PluginCrashedEvent" in the tests or
> in the example above, but would like to look into it.

Just |event instanceof PluginCrashedEvent| doesn't work?

> 
> Lastly, there seems to be a segment of code in [3] that could be replaced by
> the PluginCrashedEvent, but I wasn't sure if 1) the segment of code could be
> replaced by PluginCrashedEvent and 2) how to replace it if it could. So I
> did not touch [3] and [4] ([4] is an example of where it seems to be
> utilized on the front end).

Hm, what is the problem there?
> Just |event instanceof PluginCrashedEvent| doesn't work?

Don't think so. When adding the following check to [1]:
>     if (!(aEvent instanceof PluginCrashedEvent))
> >      return;

I get the following output when running `./mach mochitest-browser browser/base/content/test/plugins/browser_CTP_crashreporting.js`:
> JavaScript Error: "ReferenceError: PluginCrashedEvent is not defined"

Not sure if this is correct, but it seems like `PluginCrashedEvent` isn't defined in the namespace (in this case, I think that would be something like window? So there is no window.PluginCrashedEvent).

> Hm, what is the problem there?

Just wasn't sure if [2] could be replaced with `PluginCrashedEvent`. I think it can, I'll investigate a bit more.

[1] http://dxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#802
[2] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#3505
I added in checks on the front end to determine if the event was of the type `PluginCrashedEvent`. 

Still get the same "TEST-SKIPPED" notice when running [1], even after adding 
>> ac_add_options --enable-crashreporter
to my mozconfig.

I ran all the tests again and it looked like every one passed (excluding [1]).
Attachment #8497372 - Attachment is obsolete: true
Comment on attachment 8499955 [details] [diff] [review]
1045100-unify-plugin-crashed-4.diff

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

Sorry for the delay, we had a long weekend here.
Thanks for figuring this out, this looks pretty good to me! There is mostly comments on small adjustments and whitespace below.

When you addressed the comments, could you split this into two patches?
One with the webidl file, moz.build and the two C++ files for which i think you can request from smaug / Olli Pettay.
One with the other changes, for which we can try asking mconley for review (he has looked into the browser plugins code for e10s recently).

Do you have access to try, so you can push your changes there and see if there is test fallout?

::: browser/base/content/browser-plugins.js
@@ +427,5 @@
>    // are dispatched to individual plugin instances.
>    pluginCrashed : function(subject, topic, data) {
> +    if (!(subject instanceof PluginCrashedEvent))
> +      return;
> +    

Nit: Trailing whitespace.

::: browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  let gTestBrowser = null;
>  
>  let propBagProperties = {

Lets rename this to something like "eventProperties" or "crashedEventProperties".

@@ +36,5 @@
>  
>  function generateCrashEvent() {
>    let window = gTestBrowser.contentWindow;
> +  let crashedEvent = new window.PluginCrashedEvent("PluginCrashed", propBagProperties);
> +  

Whitespace.

@@ +49,4 @@
>    for (let [name, val] of Iterator(propBagProperties)) {
>      let type = typeof val;
> +    let propVal = event[name];
> +    

Whitespace.

::: browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js
@@ +61,2 @@
>          sendAsyncMessage("test:crash-plugin:crashed", {
>            crashID: crashID,

Just directly use event.pluginDumpID here?

::: browser/modules/PluginContent.jsm
@@ +782,2 @@
>        return;
> +    

Whitespace.

@@ +795,5 @@
>        // For GMP crashes we don't get a browser dump.
>      }
>  
>      try {
> +      gmpPlugin = aEvent.gmpPlugin;

The gmpPlugin property defaults to false, so we dont need the try/catch here anymore.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +327,5 @@
>    if (!doc) {
>      NS_WARNING("Couldn't get document for PluginCrashed event!");
>      return NS_OK;
>    }
> +  

Whitespace.

@@ +336,5 @@
> +  init.mPluginFilename = mPluginFilename;
> +  init.mSubmittedCrashReport = mSubmittedCrashReport;
> +  init.mBubbles = true;
> +  init.mCancelable = true;
> +  

Whitespace.

::: dom/plugins/test/mochitest/test_crash_notify.xul
@@ +64,5 @@
>    is(aEvent.type, "PluginCrashed", "event is correct type");
>  
>    var pluginElement = document.getElementById("plugin1");
>    is (pluginElement, aEvent.target, "Plugin crashed event target is plugin element");
> +  

Whitespace.

@@ +73,1 @@
>    isnot(pluginDumpID, "", "got a non-empty dump ID");

You could just directly compare with the event properties now, no need for the local variables i think.

@@ +79,3 @@
>    // The app itself may or may not have decided to submit the report, so
>    // allow either true or false here.
>    ok((didReport == true || didReport == false), "event said crash report was submitted");

I think this should actually check that we have a submittedCrashReport property and that it is a boolean.

::: dom/plugins/test/mochitest/test_crash_notify_no_report.xul
@@ +67,5 @@
>    is(aEvent.type, "PluginCrashed", "event is correct type");
>  
>    var pluginElement = document.getElementById("plugin1");
>    is (pluginElement, aEvent.target, "Plugin crashed event target is plugin element");
> +  

Whitespace.

@@ +73,2 @@
>       "plugin crashed event has the right interface");
> +  

Whitespace.

@@ +73,3 @@
>       "plugin crashed event has the right interface");
> +  
> +  var pluginName = aEvent.pluginName;

No need for the local var here?

@@ +78,3 @@
>    // The app itself may or may not have decided to submit the report, so
>    // allow either true or false here.
>    ok((didReport == true || didReport == false), "event said crash report was submitted");

The same applies here as in test_crash_notify.xul

::: dom/plugins/test/mochitest/test_hangui.xul
@@ +115,5 @@
>    ok(true, "Plugin crashed notification received");
>    is(aEvent.type, "PluginCrashed", "event is correct type");
>  
>    is(p, aEvent.target, "Plugin crashed event target is plugin element");
> +  

Whitespace.

@@ +121,3 @@
>       "plugin crashed event has the right interface");
>  
> +  var pluginDumpID = aEvent.pluginDumpID;

I dont think we need the local variables here?

@@ +130,3 @@
>    // The app itself may or may not have decided to submit the report, so
>    // allow either true or false here.
>    ok((didReport == true || didReport == false), "event said crash report was submitted");

Again, the check here should be adjusted too.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1786,5 @@
>    if (!doc) {
>      NS_WARNING("Couldn't get document for PluginCrashed event!");
>      return true;
>    }
> +  

Whitespace.

@@ +1790,5 @@
> +  
> +  PluginCrashedEventInit init;
> +  init.mPluginDumpID = aPluginDumpID;
> +  init.mPluginName = aPluginName;
> +  init.mGmpPlugin = Nullable<bool>(true);

You can just do |init.mGmpPlugin.SetValue(true)|.

@@ +1794,5 @@
> +  init.mGmpPlugin = Nullable<bool>(true);
> +  init.mSubmittedCrashReport = false;
> +  init.mBubbles = true;
> +  init.mCancelable = true;
> +  

Whitespace.

@@ +1797,5 @@
> +  init.mCancelable = true;
> +  
> +  nsRefPtr<PluginCrashedEvent> event =
> +    PluginCrashedEvent::Constructor(doc, NS_LITERAL_STRING("PluginCrashed"), init);
> +  

Whitespace.
Attachment #8499955 - Flags: feedback+
Thanks for the review! And sorry for the delay. Almost ready with the changes (still getting familiar with mercurial).

Couple of things:
1. In regards to 
> I think this should actually check that we have a submittedCrashReport property and that it is a boolean.

You're saying the code should look something like 
> ok((typeof aEvent.submittedCrashReport == "boolean" && aEvent.submittedCrashReport == true), "event said crash report was submitted");

2. Was there a reason `let` was used to create numerous local vars in the first place? Just curious if it was necessary when using the custom event.

3. I don't think I have access to try and I'm not particularly familiar with how it works but I'd be more than happy to read up on it and learn how to use it.

(Apologies for all the whitespace. Just started using XCode, not very familiar with using it yet).
This patch contains the moz.build, web.idl and 2 cpp files.
Attachment #8499955 - Attachment is obsolete: true
This patch contains the front end code (*.js, *.xul, *.jsm).

Regarding:
> I think this should actually check that we have a submittedCrashReport property and that it is a boolean.

That change isn't included in this patch, wanted to make sure I understood your comment (see [1]).

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1045100#c25
(In reply to Connor [:cojo] from comment #25)
> 1. In regards to 
> > I think this should actually check that we have a submittedCrashReport property and that it is a boolean.
> 
> You're saying the code should look something like 
> > ok((typeof aEvent.submittedCrashReport == "boolean" && aEvent.submittedCrashReport == true), "event said crash report was submitted");

I would do two separate checks here, one for |"submittedCrashReport" in aEvent| and one ofr |typeof aEvent.submittedCrashReport == "boolean"|.
As the comment there points out, we cant count on the value being stable.

> 
> 2. Was there a reason `let` was used to create numerous local vars in the
> first place? Just curious if it was necessary when using the custom event.

I am not sure what specific location you refer to, but most of the places that used the event simply required to write more, which was made more readable by using local vars, e.g.:

> event.detail.QueryInterface(Ci.nsIPropertyBag2).getPropertyAsAString("pluginDumpID");

vs.

> event.pluginDumpID;


> 3. I don't think I have access to try and I'm not particularly familiar with
> how it works but I'd be more than happy to read up on it and learn how to
> use it.

Ah, i can push it to try for you then. After you landed a few patches, you should consider requesting access to push to try - its pretty convenient to quickly check whether you break some of all our platforms and test-suites yourself.
Attachment #8503672 - Flags: review?(bugs)
(In reply to Connor [:cojo] from comment #27)
> Created attachment 8503674 [details] [diff] [review]
> 1045100-unify-plugin-crashed-frontend.diff
> 
> This patch contains the front end code (*.js, *.xul, *.jsm).
> 
> Regarding:
> > I think this should actually check that we have a submittedCrashReport property and that it is a boolean.
> 
> That change isn't included in this patch, wanted to make sure I understood
> your comment (see [1]).

Lets fix that per the above comment, then i will push to try and do a - hopefully final - review pass.
Comment on attachment 8503672 [details] [diff] [review]
1045100-unify-plugin-crashed-internal.diff

>+[Constructor(DOMString type, optional PluginCrashedEventInit eventInitDict), ChromeOnly]
>+interface PluginCrashedEvent : Event
>+{
>+  readonly attribute DOMString pluginDumpID;
>+  readonly attribute DOMString pluginName;
>+  readonly attribute DOMString? browserDumpID;
>+  readonly attribute DOMString? pluginFilename;
>+  readonly attribute boolean submittedCrashReport;
>+  readonly attribute boolean? gmpPlugin;
>+};
>+
>+dictionary PluginCrashedEventInit : EventInit
>+{
>+  DOMString pluginDumpID = "";
>+  DOMString pluginName = "";
>+  DOMString? browserDumpID = "";
>+  DOMString? pluginFilename = "";
Why these are nullables, but default to ""?
Perhaps default to null?

>+  boolean submittedCrashReport = false;
>+  boolean? gmpPlugin = false;
Does this really need to be nullable? And if so, why to default to false?
I'd make it boolean gmpPlugin = false;
>+
>+  init.mGmpPlugin.setValue(true);
...and then you could just assign true here.
(and btw, does setValue actually compile? Shouldn't it have been SetValue)
Attachment #8503672 - Flags: review?(bugs) → review-
This patch has the checks recommended in [1].

One thing I was curious about was the necessity for the |"submittedCrashReport" in aEvent| check. I was under the assumption (based on some educated guessing) that the |PluginCrashedEventInit| dictionary acted as a set of default values whenever a |PluginCrashedEvent| is created on the frontend. Given the check |"submittedCrashReport" in aEvent| is it correct to say that |PluginCrashedEventInit| does not act as a set of default values? (Note that my knowledge of .webidl is limited to what I've picked up in this patch. Still working on understanding it).

> (and btw, does setValue actually compile? Shouldn't it have been SetValue)

Good point. It does not (did not compile to check after I made the change, my apologies). The issue now is that SetValue does not compile either and gives the following: 
> error: member reference base type 'bool' is not a structure or union

Was going to work on figuring that out and upload the other patch with the recommended changes.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1045100#c28
Attachment #8503674 - Attachment is obsolete: true
Ha, took another look. I had made the change smaug suggested regarding |mGmpPlugin| but forgot I had done so (making it just a boolean, not nullable, and defaulting to false). That's what was throwing 

> error: member reference base type 'bool' is not a structure or union

I don't think there's any issue with making it just a boolean, I'll finish up implementing and testing the rest of smaugs recommendations and submit another version of the "internal" patch.
Uploaded a new patch with the changes smaug recommended.

> Why these are nullables, but default to ""?
Makes sense. First time working with .webidl so I ended up following
> "" should be fine for DOMString property
without much thought.

> Does this really need to be nullable? And if so, why to default to false?
I don't think it needs nullable so I changed it accordingly. I guess it could be an issue if there was some code that depended on whether or not the |mGmpPlugin| property existed on the object (not just whether it was true or false) but I don't think such code exists in the core codebase (at least nothing I could find, outside of [1]. But that's on the frontend and setting |mGmpPlugin| to false by default wouldn't affect it)

[1] http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#820
Attachment #8503672 - Attachment is obsolete: true
(In reply to Connor [:cojo] from comment #33)
> > Does this really need to be nullable? And if so, why to default to false?
> I don't think it needs nullable so I changed it accordingly. I guess it
> could be an issue if there was some code that depended on whether or not the
> |mGmpPlugin| property existed on the object (not just whether it was true or
> false) but I don't think such code exists in the core codebase (at least
> nothing I could find, outside of [1]. But that's on the frontend and setting
> |mGmpPlugin| to false by default wouldn't affect it)

Indeed, that is much better and the frontend will be fine with it, lets do this.
nullable property does exists in the interface (and in the prototype of the object implementing the interface). nullable just means its value can be null.
Ah right, that makes sense. This patch [1] should have what I think are the right changes regarding the above (nullables set to null, mGmpPlugin set to boolean, not nullable).

[1] https://bug1045100.bugzilla.mozilla.org/attachment.cgi?id=8506623

(I'll look into requesting access to try too).
If that patch addresses the issues mentioned above, you can go ahead and request review again.

Let me know if i can push something to try for you.
Attachment #8506557 - Flags: review?(georg.fritzsche)
Attachment #8506623 - Flags: review?(georg.fritzsche)
Great. I flagged both patches for review.
Attachment #8506623 - Flags: review?(georg.fritzsche) → review?(bugs)
Comment on attachment 8506557 [details] [diff] [review]
1045100-unify-plugin-crashed-frontend-2.diff

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

::: browser/modules/PluginContent.jsm
@@ +791,3 @@
>  
>      try {
> +      browserDumpID = aEvent.browserDumpID;

No need for a try-catch here anymore either - just assign directly at |let browserDumpID ...|.

::: dom/plugins/test/mochitest/test_crash_notify.xul
@@ +70,5 @@
>       "plugin crashed event has the right interface");
>  
> +  isnot(aEvent.pluginDumpID, "", "got a non-empty dump ID");
> +  is(aEvent.pluginName, "Test Plug-in", "got correct plugin name");
> +  isnot(aEvent.pluginFilename, "", "got a non-empty filename");

As this property is nullable, we should add a check for either a) the type being a string or b) the value not being null.

@@ +75,4 @@
>    // The app itself may or may not have decided to submit the report, so
>    // allow either true or false here.
> +  ok(("submittedCrashReport" in aEvent && typeof aEvent.submittedCrashReport == "boolean"),
> +      "event said crash report was submitted");

We don't need to cramp this in one check, breaking it into two would be more readable.

::: dom/plugins/test/mochitest/test_crash_notify_no_report.xul
@@ +76,4 @@
>    // The app itself may or may not have decided to submit the report, so
>    // allow either true or false here.
> +  ok((aEvent.submittedCrashReport == true || aEvent.submittedCrashReport == false),
> +      "event said crash report was submitted");

This should be changed like the check in test_crash_notify.xul.

::: dom/plugins/test/mochitest/test_hangui.xul
@@ +121,5 @@
>       "plugin crashed event has the right interface");
>  
> +  isnot(aEvent.pluginDumpID, "", "got a non-empty dump ID");
> +  is(aEvent.pluginName, "Test Plug-in", "got correct plugin name");
> +  isnot(aEvent.pluginFilename, "", "got a non-empty filename");

As this property is nullable, we should add a check for either a) the type being a string or b) the value not being null.

@@ +126,4 @@
>    // The app itself may or may not have decided to submit the report, so
>    // allow either true or false here.
> +  ok((aEvent.submittedCrashReport == true || aEvent.submittedCrashReport == false),
> +      "event said crash report was submitted");

This should be changed like the check in test_crash_notify.xul.
Attachment #8506557 - Flags: review?(georg.fritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #39)
> Comment on attachment 8506557 [details] [diff] [review]
> 1045100-unify-plugin-crashed-frontend-2.diff
> 
> Review of attachment 8506557 [details] [diff] [review]:
> -----------------------------------------------------------------

I should add that this is nearly done!
We just need to fix the smaller remaining issues in the comment above.
These patches don't apply cleanly anymore - please rebase them on a current revision of mozilla-central, then i will push this to try.
Attachment #8506623 - Flags: review?(bugs) → review+
I made the above changes and then ran |hg pull --rebase| which did not indicate any errors/issues. If the patch doesn't apply cleanly should I expect to see some errors/issues when running |hg pull --rebase|? (Also, is |hg pull --rebase| the correct command to run?).
Attachment #8506623 - Attachment is obsolete: true
Attachment #8509462 - Flags: review?(georg.fritzsche)
Attachment #8506557 - Attachment is obsolete: true
Attachment #8509463 - Flags: review?(georg.fritzsche)
I wasn't sure if the |hg pull --rebase| was correct so I cloned mozilla-central and applied the patches on top of it (and fixed any issues with them being applied cleanly) and set them to be reviewed (hopefully that's the correct step).
Comment on attachment 8509462 [details] [diff] [review]
1045100-unify-plugin-crashed-internal-3.diff

Interdiff shows basically no changes:
http://benjamin.smedbergs.us/interdiff/interdiff.php?patch1url=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8506623&patch2url=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8509462

... and smaug already gave r+, so no review needed from me or him.
Attachment #8509462 - Flags: review?(georg.fritzsche) → review+
Attachment #8509463 - Flags: review?(georg.fritzsche) → review+
(In reply to Connor [:cojo] from comment #45)
> I wasn't sure if the |hg pull --rebase| was correct so I cloned
> mozilla-central and applied the patches on top of it (and fixed any issues
> with them being applied cleanly) and set them to be reviewed (hopefully
> that's the correct step).

Thanks, that was exactly right (only that one patch didn't need review anymore).

I pushed the patches to try now:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84e2147e7075
https://tbpl.mozilla.org/?tree=Try&rev=84e2147e7075
(In reply to Georg Fritzsche [:gfritzsche] from comment #47)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84e2147e7075
> https://tbpl.mozilla.org/?tree=Try&rev=84e2147e7075

This is failing on all Window platforms with failures like:
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_busy_hang.xul | plugin crashed event has the right interface - expected PASS
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_busy_hang.xul | uncaught exception - TypeError: aEvent.detail is undefined at http://mochi.test:8888/chrome/dom/plugins/test/mochitest/hang_test.js:98
...

See e.g.:
https://tbpl.mozilla.org/php/getParsedLog.php?id=50790966&tree=Try&full=1

Can you check what's going on?
Flags: needinfo?(cojojennings)
Yes. I'm not sure if this will fix all the issues (I will look into them more thoroughly) but I can see that [1] will need to be modified to use PluginCrashedEvent instead of DOMCustomEvent. I will make those modifications and attach a new patch.

I have not yet looked into this very thoroughly but it seems odd the error only triggered on Windows platforms. It seems like [1] would cause problems on all platforms (Unless hang_test.js only gets executed on Windows platforms).

[1] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/hang_test.js#87
Right, some tests indeed get only run on Windows, including test_busy_hang.xul:
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/chrome.ini#11,24
Makes sense. So adding

> ac_add_options --enable-crashreporter

to my mozconfig wouldn't help since I'm not on a Windows machine. Should have realized that earlier, my mistake. I think I can test if I comment out that line in chrome.ini on my local machine (and not adding that change to the patch).

Anyway, I'll fix up where that test is failing and push the patch.
Flags: needinfo?(cojojennings)
This should update [1] so that it works with the PluginCrashedEvent. All the updates were made to [2] (which is included as a script in [1]). 

I don't have a windows platform so I wasn't able to confirm that the changes pass. I did try commenting out [3] on my local machine to test but it ended up failing both with my changes and when I ran a build without them (I don't have a Windows machine, so I assume that's why it failed).

[1] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_busy_hang.xul
[2] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/hang_test.js
[3] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/chrome.ini#11
Attachment #8509463 - Attachment is obsolete: true
Attachment #8511591 - Flags: review?(georg.fritzsche)
Comment on attachment 8511591 [details] [diff] [review]
1045100-unify-plugin-crashed-frontend-4.diff

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

Thanks, this looks good.
Note that nsObjectLoadingContent.cpp was just moved from content/base/src to dom/base, so you should rebase to a new mozilla-central.

I pushed this to try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0662db27069e
https://tbpl.mozilla.org/?tree=Try&rev=0662db27069e

::: dom/plugins/test/mochitest/hang_test.js
@@ +96,3 @@
>       "plugin crashed event has the right interface");
>  
> +  isnot(aEvent.pluginDumpID, "", "got a non-empty dump ID");

Also check that this is a string (e.g. think of what this check does when this property is null).
Attachment #8511591 - Flags: review?(georg.fritzsche) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #53)
> Note that nsObjectLoadingContent.cpp was just moved from content/base/src to
> dom/base, so you should rebase to a new mozilla-central.

Of course this affects the "internal", not the "frontend" patch.
> Of course this affects the "internal", not the "frontend" patch.

Yep, of course.

> Also check that this is a string (e.g. think of what this check does when this property is null).

|pluginDumpID| is just a DOMString and not a DOMString?. It still needs to be checked for null?
(In reply to Connor [:cojo] from comment #55)
> > Also check that this is a string (e.g. think of what this check does when this property is null).
> 
> |pluginDumpID| is just a DOMString and not a DOMString?. It still needs to
> be checked for null?

Right, it's not overly important there - but someone may change it to DOMString? or we start using something other than this WebIDL etc., so it would be good to assert this as well.
Makes sense. I'm assuming it also makes sense to have the same check for whether |pluginDumpID| is null wherever we have a test for it (test_crash_notify.xul, test_hangui.xul, etc)?

(Also, I assume it would make sense to add checks for |pluginName| as well?)

I'll put those checks in and push a new patch.

(And rebase the "internal" patch).
(In reply to Connor [:cojo] from comment #57)
> Makes sense. I'm assuming it also makes sense to have the same check for
> whether |pluginDumpID| is null wherever we have a test for it
> (test_crash_notify.xul, test_hangui.xul, etc)?
> 
> (Also, I assume it would make sense to add checks for |pluginName| as well?)

Sure, sounds sensible. Nothing pressing, but nice to have.

There is one failure on try on Windows XP opt, the rest looks good:
https://tbpl.mozilla.org/php/getParsedLog.php?id=51122400&tree=Try&full=1
This patch includes new type checks for |pluginDumpID|, |pluginName| and |pluginFilename|. 

Can you run this through Try? I'd like to make sure the tests are good and I'm curious to see if the timeout failure occurs again on Windows XP.
Attachment #8511591 - Attachment is obsolete: true
Attachment #8512800 - Flags: review?(georg.fritzsche)
I rebased this patch and nsObjectLoadingContent.cpp should now be in the correct place (under dom/base).
Attachment #8509462 - Attachment is obsolete: true
> There is one failure on try on Windows XP opt, the rest looks good

Doesn't look like this failure occurred in the last push to try. Given that the failure appeared to have been a timeout, it is possible that it's not related to this patch?
Right, this may just be the intermittent failure already filed before.
I am pretty busy this week, i'll try to take a final pass here later today or monday.
> Right, this may just be the intermittent failure already filed before.
Makes sense.

> I am pretty busy this week, i'll try to take a final pass here later today or monday.
Sounds good.
Comment on attachment 8512802 [details] [diff] [review]
1045100-unify-plugin-crashed-internal-4.diff

This was already reviewed - please carry over the r+ so its clear which patches are done :)
Attachment #8512802 - Flags: review+
Comment on attachment 8512800 [details] [diff] [review]
1045100-unify-plugin-crashed-frontend-5.diff

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

This looks good to go per try.
Thanks for staying on this over the long turn-arounds here :)
Attachment #8512800 - Flags: review?(georg.fritzsche) → review+
Please upload patches with the commit message updated to the format recommended here:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

... carry over the review+ to the new bugs, then go ahead and add the keyword "checkin-needed" (so someone will land them).
Flags: needinfo?(cojojennings)
(In reply to Connor [:cojo] from comment #22)
> Just wasn't sure if [2] could be replaced with `PluginCrashedEvent`. I think
> it can, I'll investigate a bit more.
[...]
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.
> cpp#3505

Hm, actually, what happened to this part?
browser.js listens to the event fired there and passes it on to gPluginHandler.pluginCrashed:
http://hg.mozilla.org/mozilla-central/annotate/0b81c10a9074/browser/base/content/browser.js#l899
... which i think doesn't expect property bags anymore as of this patch.
Good question.

I think I was just confused about whether the areas on the frontend that listen for "plugin-crashed" [1] needed to be modified (as opposed to those that listen for "PluginCrashed" [2]). When I began working on the patch I tried changing the areas that used "plugin-crashed" to use "PluginCrashedEvent" it seemed to cause a number of issues (can't remember the specifics at the moment, but I think that's why I brought it up in [3]).

I'll modify nsPluginHost.cpp to use "PluginCrashedEvent" and modify the areas of the frontend that listen for "plugin-crashed" and push new versions of the patches.

[1] http://dxr.mozilla.org/mozilla-central/search?q=%22plugin-crashed
[2] http://dxr.mozilla.org/mozilla-central/search?q="PluginCrashed"
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1045100#c20
Flags: needinfo?(cojojennings)
Ok, that sounds good. Per [1] "plugin-crashed" is called on the pluginCrashed method in browser-plugins.js that your frontend patch changes to use the PluginCrashedEvent.
This means that we will throw here and not autosubmit the crash report in some scenarios.

It's sad that none of our tests covered this, we should file a follow-up bug on that.

[1] http://hg.mozilla.org/mozilla-central/annotate/0b81c10a9074/browser/base/content/browser.js#l899
This updated patch modifies |nsPluginHost::PluginCrashed| to use the new |PluginCrashedEvent|. 

Before this patch |nsPluginHost::PluginCrashed| did not create a custom event like |PeerConnectionImpl::PluginCrash| [1].

> nsRefPtr<PluginCrashedEvent> event =
> PluginCrashedEvent::Constructor(currentdocument, NS_LITERAL_STRING("PluginCrashed"), init);

Because |nsPluginHost::PluginCrashed| did not use a custom event, I added the above line to the patch. I used |mCurrentDocument| since it's used in |nsPluginHost::SetUpPluginInstance|.

> init.mPluginName = NS_ConvertUTF8toUTF16(crashedPluginTag->mName);

I'm not sure this is correct. |mPluginName| is a nsString and |crashedPluginTag->mName) is a nsCString. |init.mPluginName = crashedPluginTag->mName| caused a compilation error. I'm not familiar with nsString and nsCString, so I'm unsure if using |ConvertUTF8toUTF16| is correct (it did compile when using |ConvertUTF8toUTF16|).

[1] http://hg.mozilla.org/mozilla-central/file/72ae882fce1a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#l1793
Attachment #8512802 - Attachment is obsolete: true
Attachment #8517681 - Flags: review?(georg.fritzsche)
These are the changes for the frontend after modifying |nsPluginHost::PluginCrashed|. 

Note I modified 2 new files [1] and [2] since they'll now use |PluginCrashedEvent| instead of a propBag.

If you could push these to try that'd be great. I ran most of the tests on my local and there didn't seem to be any major issues.

[1] http://hg.mozilla.org/mozilla-central/file/b6cd2dd85b26/testing/specialpowers/content/SpecialPowersObserverAPI.js#l86
[2] http://hg.mozilla.org/mozilla-central/file/b6cd2dd85b26/layout/tools/reftest/reftest.js#l1982=
Attachment #8512800 - Attachment is obsolete: true
Attachment #8517687 - Flags: review?(georg.fritzsche)
Comment on attachment 8517687 [details] [diff] [review]
1045100-unify-plugin-crashed-frontend-6.diff

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

This looks good, there is mostly an issue with the SpecialPowersObserverAPI.js below.

I shouldn't do final review on these two:
layout/tools/reftest/reftest.js
testing/specialpowers/content/SpecialPowersObserverAPI.js

Could you move those to another patch for test harness changes? I think jmaher could review that.

::: dom/plugins/test/mochitest/test_crash_notify.xul
@@ +34,3 @@
>      let directoryService =
> +    Components.classes["@mozilla.org/file/directory_service;1"].
> +    getService(Components.interfaces.nsIProperties);

No need to change anything on those two lines.

::: dom/plugins/test/mochitest/test_crash_notify_no_report.xul
@@ +37,5 @@
>      let pendingD = directoryService.get("UAppData",
>                                          Components.interfaces.nsIFile);
>      pendingD.append("Crash Reports");
>      pendingD.append("pending");
>      let dumpFile = pendingD.clone();    

While we touch the neighbouring line already, can you please remove the trailing whitespace here?

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +97,5 @@
>          message.dumpIDs.push({id: id, extension: "extra"});
>        }
>      }
>  
> +    function addDumpIDToMessage(propertyName) {

This looks nearly like a copy of the above function.
We can continue to use one |addDumpIDToMessage()| function if we pass the id in directly.
Note that getting the ids from the plugin event is not expected to throw, so we can just do |addDumpIDToMessage(aSubject.pluginDumpID)| etc.

@@ +129,4 @@
>          break;
>      }
> +
> +    this._sendAsyncMessage("SPProcessCrashService", message);

This changes the behavior here - previously we would only _sendAsyncMessage() for the topics "plugin-crashed" & "ipc:content-shutdown", now we always send it.
Lets keep the previous behavior here.
Attachment #8517687 - Flags: review?(georg.fritzsche) → feedback+
Comment on attachment 8517681 [details] [diff] [review]
1045100-unify-plugin-crashed-internal-5.diff

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

::: dom/plugins/base/nsPluginHost.cpp
@@ +3724,5 @@
>    // a crashreport.
>    bool submittedCrashReport = false;
>    nsCOMPtr<nsIObserverService> obsService =
> +  mozilla::services::GetObserverService();
> +  nsCOMPtr<nsIDocument> currentdocument = do_QueryReferent(mCurrentDocument);

Minor note that you have to null-check when querying weak pointers:
https://developer.mozilla.org/en-US/docs/Weak_reference

More interestingly, that |mCurrentDocument| is weird and we shouldn't use it - it is a weak pointer to the last document for which we set up a plugin in |nsPluginHost::SetUpPluginInstance()|.

We need to do this differently.
Attachment #8517681 - Flags: review?(georg.fritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #74)
> Comment on attachment 8517681 [details] [diff] [review]
> 1045100-unify-plugin-crashed-internal-5.diff
> 
> Review of attachment 8517681 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/nsPluginHost.cpp
> @@ +3724,5 @@
> >    // a crashreport.
> >    bool submittedCrashReport = false;
> >    nsCOMPtr<nsIObserverService> obsService =
> > +  mozilla::services::GetObserverService();
> > +  nsCOMPtr<nsIDocument> currentdocument = do_QueryReferent(mCurrentDocument);
> 
> Minor note that you have to null-check when querying weak pointers:
> https://developer.mozilla.org/en-US/docs/Weak_reference
> 
> More interestingly, that |mCurrentDocument| is weird and we shouldn't use it
> - it is a weak pointer to the last document for which we set up a plugin in
> |nsPluginHost::SetUpPluginInstance()|.
> 
> We need to do this differently.

Hm, smaug, i guess the PluginCrashed event needs to go with a document and is not good for use in this context?
Flags: needinfo?(bugs)
Oh, really odd to use Event without actually dispatching it. Just leave the code there alone?
Flags: needinfo?(bugs)
> Oh, really odd to use Event without actually dispatching it. Just leave the code there alone?
You mean use nsIWritablePropertyBag instead of PluginCrashedEvent?
Alright, sorry about that Connor, lets not do this then :(
(In reply to Connor [:cojo] from comment #77)
> > Oh, really odd to use Event without actually dispatching it. Just leave the code there alone?
> You mean use nsIWritablePropertyBag instead of PluginCrashedEvent?
And I mean only in that case where we don't dispatch any event. That one case is using ObserverService
So, looking through the previous patches again, pluginCrashed() in browser-plugins.js, is only called from the "plugin-crashed" observer.
That means if we switch back to using nsIWritablePropertyBag in nsPluginHost::PluginCrashed() (i.e. the last internal patch), then we shouldn't change the pluginCrashed() function either.
With that we should be good (and tests run through fine).
> Alright, sorry about that Connor, lets not do this then :(
Heh, no worries. It was a good opportunity to work through more of the codebase.

> With that we should be good (and tests run through fine).
So the next steps are:
1) Use code in 1045100-unify-plugin-crashed-frontend-5.diff [1] and 1045100-unify-plugin-crashed-internal-4.diff [2]
2) Clean up the commit messages so they conform to https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions
3) Upload the cleaned up patches and add r+ and "checkin-needed".

Does that look correct or am I missing something?

[1] https://bug1045100.bugzilla.mozilla.org/attachment.cgi?id=8512800
[2] https://bug1045100.bugzilla.mozilla.org/attachment.cgi?id=8512802
Sorry for the delay, though i had responded here already. needinfo is always good to set for questions, then i get notifications for outstanding questions.

(In reply to Connor [:cojo] from comment #82)
> > With that we should be good (and tests run through fine).
> So the next steps are:
> 1) Use code in 1045100-unify-plugin-crashed-frontend-5.diff [1] and
> 1045100-unify-plugin-crashed-internal-4.diff [2]

We should also remove the changes in browser-plugins.js - from my tracing through the code, gPluginHandler.pluginCrashed() is only triggered through nsPluginHost::PluginCrashed() via the "plugin-crashed" notification.
Aw, misunderstood. Thought the changes to browser-plugin.js were only in the most recent .diff. I'll make that change and push the new patches.
Mike, could you help finishing up the last bit here while i'm away for the next two weeks?
The patch for the internal parts is done and had review, so this should only about the remaining small changes to the frontend patch in the previous comments.
Flags: needinfo?(mconley)
I assume you mean for me to work with / mentor Connor to drive this to completion, rather than picking up the patch myself? Either is fine, but I just want to make sure I know what I'm doing here. :)
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #86)
> I assume you mean for me to work with / mentor Connor to drive this to
> completion

Yes, indeed. Thanks :)
Mentor: mconley
Apologies for the delay.

I rolled back to the changes made in [1] and [2] (and removed the changes made to browser-plugins.js that were in [2]). I also made the changes to the commit messages based on [3].

Mike, since you're now the acting mentor, when I upload these patches should I mark them as review? or review+ (if review+, should I also mark checkin-needed?). Not sure if you want to take another look at them.

Thanks!

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8512802&action=edit
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8512800&action=edit
[3] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions
(In reply to Connor [:cojo] from comment #88)
> Mike, since you're now the acting mentor, when I upload these patches should
> I mark them as review? or review+ (if review+, should I also mark
> checkin-needed?). Not sure if you want to take another look at them.

The "internal" patch should just be rolled back to the last version. That was already reviewed, so you can add r+ on that.

The "frontend" patch needed the browser-plugins.js changes, so it should get a last look by Mike when you are done.
This is the updated "internal" patch. It is the work from [1] with a cleaned up commit message. Carrying through the "r+" for this patch.

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1045100&attachment=8512802
Attachment #8517681 - Attachment is obsolete: true
Attachment #8523475 - Flags: review+
This is the updated "frontend" patch. It has all the changes from [1] with the changes to browser-plugins.js removed.

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1045100&attachment=8512800
Attachment #8517687 - Attachment is obsolete: true
Attachment #8523476 - Flags: review?(mconley)
Comment on attachment 8523476 [details] [diff] [review]
1045100-unify-plugin-crashed-frontend-7.diff

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

This looks good - just some minor nits. We can probably get this landed today once these are fixed.

::: browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js
@@ +50,1 @@
>      let type = typeof val;

Type is not being used, and can be removed.

::: dom/plugins/test/mochitest/hang_test.js
@@ +96,5 @@
>       "plugin crashed event has the right interface");
>  
> +  is(typeof aEvent.pluginDumpID, "string", "pluginDumpID is correct type");
> +  isnot(aEvent.pluginDumpID, "", "got a non-empty dump ID");
> +  is(typeof aEvent.pluginName, "string", "pluginDumpName is correct type");

pluginName, not pluginDumpName.
Attachment #8523476 - Flags: review?(mconley)
Apologies for the delay. Didn't have time to make these changes earlier. This patch has the fixes for [1].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1045100#c92
Attachment #8523476 - Attachment is obsolete: true
Attachment #8524241 - Flags: review?(mconley)
Comment on attachment 8524241 [details] [diff] [review]
1045100-unify-plugin-crashed-frontend-8.diff

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

This looks good to me, thanks!

Pushed to try for a final run:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ef5cab7e2ce2
Attachment #8524241 - Flags: review?(mconley) → review+
Woo, super-green! Great job, Connor!
Keywords: checkin-needed
smaug is the review as dom peer still valid, because its needed here:

remote: WebIDL file dom/webidl/PluginCrashedEvent.webidl altered in changeset 6a3deb9f4f3f without DOM peer review
Flags: needinfo?(bugs)
Keywords: checkin-needed
I don't know which 6a3deb9f4f3f we're talking about here. Don't see 6a3deb9f4f3f in m-i
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/b7acf8f21edc
https://hg.mozilla.org/mozilla-central/rev/f46bc389856d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Iteration: --- → 36.3
I've been started looking this because comm-central also needs this change. So I have a question now.
There is still usage of nsIPropertyBag2.

http://hg.mozilla.org/mozilla-central/file/b7acf8f21edc/browser/base/content/browser-plugins.js#l458

Is there any follow-up bugs to fix those or those codes are dead codes?
Flags: needinfo?(mconley)
Depends on: 1102588
(In reply to Hiroyuki Ikezoe (:hiro) from comment #100)
> I've been started looking this because comm-central also needs this change.
> So I have a question now.
> There is still usage of nsIPropertyBag2.
> 
> http://hg.mozilla.org/mozilla-central/file/b7acf8f21edc/browser/base/content/
> browser-plugins.js#l458
> 
> Is there any follow-up bugs to fix those or those codes are dead codes?

It looks like that function listens for an observer notification for "plugin-crashed" that is fired here:

http://hg.mozilla.org/mozilla-central/file/8c02f3280d0c/dom/plugins/base/nsPluginHost.cpp#l3755

and that subject of that notification is an nsIWritablePropertyBag2, so I think we're OK.

I think our hands are kind of tied with nsIObserverService, since we have to pass something that implements nsISupports through it as the subject. I imagine this is why we're still using the nsIPropertyBag2 here.

But then again, I just reviewed the front-end bits of this patch. I'll needinfo gfritzsche to make sure.
Flags: needinfo?(mconley) → needinfo?(georg.fritzsche)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #101)
> It looks like that function listens for an observer notification for
> "plugin-crashed" that is fired here:
> 
> http://hg.mozilla.org/mozilla-central/file/8c02f3280d0c/dom/plugins/base/
> nsPluginHost.cpp#l3755
> 
> and that subject of that notification is an nsIWritablePropertyBag2, so I
> think we're OK.
> 
> I think our hands are kind of tied with nsIObserverService, since we have to
> pass something that implements nsISupports through it as the subject. I
> imagine this is why we're still using the nsIPropertyBag2 here.

The main problem (and the last rollback on the patch here) was because we don't necessarily have a document to work on on that path, see comment 75.
Flags: needinfo?(georg.fritzsche)
You need to log in before you can comment on or make changes to this bug.