1046052 - Create mochitest-browser test for GMP crash reporting

Status: NEW

(Firefox :: General, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

We need to properly test the full code path for GMP crash reporting, up to the notification being shown and the crash being submitted.

When bug 1044408 is resolved we can actually crash GMP plugins from testing and should be able to base the notification and crash event checks on the existing NPAPI ones.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Starting points:
Bug 1037125 has automated tests using the fake GMP, bug 1044408 added a pref for crashing them.
Bug 1009765 has test coverage for crashing and FHR collection of the resulting data.

We need to ensure that after a crash:
* the crash notification bar is shown and contains the report button
* the report button is functional and submits successfully

Comment 2

[Georg Fritzsche [:gfritzsche]]

Gavin, this is also one of the last OpenH264 bugs that we should include the next iteration.

Comment 3

[Marco Mucci [:MarcoM]]

Hi Georg, Gavin is not arriving for the week week until late tonight so I'm addin Bug 1046052 to 35.1 for him until he can review it as well.

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> Gavin, this is also one of the last OpenH264 bugs that we should include the
> next iteration.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Qeole will probably look into this \o/

Comment 5

[Georg Fritzsche [:gfritzsche]]

Ok, we have this test already for the notification bar for plugin crashes:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js

We should duplicate & adopt this to OpenH264 with starting points from comment 1.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Note that OpenH264 plugin crashes are not associated with any in-page elements, so we don't need to worry about any test coverage like that.

Comment 7

[Qeole]

Hello there,

Sadly I've been unable to find enough time to get to something here, and probably won't be able any more in coming weeks. I'm unassigning for now.

Comment 8

[Qeole]

Attachment: gmp_crashinfobar.patch

Could find some time after all.

Based on code from fake plugin test [1] and browser_globalplugin_crashinfobar.js [2], I wrote a first patch.
It is a browser chrome test, and does the following:
1. opens page plugin_fake_h264.html, which calls the fake plugin;
2. crashes the fake plugin thanks to pref set in bug 1044408;
3. checks that crash infobar is displayed with correct message.

It does not check anything related to the report button yet (not even whether it's shown or not).

Georg, does it correspond so far to what you had in mind?

[1] https://bug1037125.bugzilla.mozilla.org/attachment.cgi?id=8458273
[2] http://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8507509 [details] [diff] [review]
gmp_crashinfobar.patch

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

Thanks for figuring this out, this looks promising.

Most generally, we do have better tools for async testing now (promises, generators, test utilities like add_task()), so i would implement this a little differently to get around the callbacks etc.
A good recent example would be browser_CTP_remove_navigate.js.

::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
@@ +54,5 @@
> +  // Again we have to wait for fake plugin to launch before we crash it.
> +  // "executeSoon()" won't do the trick.
> +  setTimeout(() => {
> +    Services.prefs.setBoolPref("media.gmp.plugin.crash", true);
> +  }, 500);

Hm, i wonder if we have at least a better way for this now. Maybe drno knows?

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> ::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
> @@ +54,5 @@
> > +  // Again we have to wait for fake plugin to launch before we crash it.
> > +  // "executeSoon()" won't do the trick.
> > +  setTimeout(() => {
> > +    Services.prefs.setBoolPref("media.gmp.plugin.crash", true);
> > +  }, 500);
> 
> Hm, i wonder if we have at least a better way for this now. Maybe drno knows?

Nils, do you know of a better solution then an arbitrary timeout to wait for the (fake) OpenH264 plugin to instantiate in tests?

Comment 11

[Nils Ohlmeier [:drno]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> > ::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
> > @@ +54,5 @@
> > > +  // Again we have to wait for fake plugin to launch before we crash it.
> > > +  // "executeSoon()" won't do the trick.
> > > +  setTimeout(() => {
> > > +    Services.prefs.setBoolPref("media.gmp.plugin.crash", true);
> > > +  }, 500);
> > 
> > Hm, i wonder if we have at least a better way for this now. Maybe drno knows?
> 
> Nils, do you know of a better solution then an arbitrary timeout to wait for
> the (fake) OpenH264 plugin to instantiate in tests?

Unfortunately not really. For tests with the real plugin I was hopping that I could somehow query the status of installed add-ons. But I haven't really looked at the API, so I don't know of they provide you with some callback or event at all.

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Nils Ohlmeier [:drno] from comment #11)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> > (In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> > > ::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
> > > @@ +54,5 @@
> > > > +  // Again we have to wait for fake plugin to launch before we crash it.
> > > > +  // "executeSoon()" won't do the trick.
> > > > +  setTimeout(() => {
> > > > +    Services.prefs.setBoolPref("media.gmp.plugin.crash", true);
> > > > +  }, 500);
> > > 
> > > Hm, i wonder if we have at least a better way for this now. Maybe drno knows?
> > 
> > Nils, do you know of a better solution then an arbitrary timeout to wait for
> > the (fake) OpenH264 plugin to instantiate in tests?
> 
> Unfortunately not really. For tests with the real plugin I was hopping that
> I could somehow query the status of installed add-ons.

Currently we only have one "register plugin" function on mozIGeckoMediaPluginService (addPluginDirectory), that's it.
The addon manager could reflect when it registers/unregister GMP plugins, but it doesn't know
* if addPluginDirectory failed (unless it fails before being moved to the off-main-thread task)
* when addPluginDirectory is finished
I will file something on fixing addPluginDirectory.

However, once the plugin is registered and we initiate a test connection etc. (see plugin_fake_h264.html in the test), there must be a point where the plugin is definitely loaded (and in use)?

Comment 13

[Qeole]

Attachment: gmp_crashinfobar.patch (v2)

Here's a new patch.
I did not perform any refactoring for the async events (so I did not address comments 9 to 12), since I'm waiting for bug 1087246 to be fixed.

Instead I focused on remaining (sub)tests: the test now checks that buttons are correctly displayed, then clicks “Submit a report” button and checks that report is correctly submitted.
All subtests pass successfully on my computer.

(New code is based on dom/plugins/test/mochitest/test_crash_submit.xul,
http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_crash_submit.xul)

Georg, could you please provide some feedback again?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8515715 [details] [diff] [review]
gmp_crashinfobar.patch (v2)

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

The approach here looks good, thanks!

Two general comments that affect the whole test:
* I think we can already make this a promise-/task-based test (using add_task() etc.). This will make it much more readable/cleaner.
* We should use the Assert.jsm functions for new test code instead of is(), ok(), etc. For mochitests, those functions are already imported.

I just did high-level comments at this point.

::: browser/base/content/test/plugins/browser.ini
@@ +69,5 @@
>  [browser_CTP_resize.js]
>  [browser_CTP_zoom.js]
>  [browser_globalplugin_crashinfobar.js]
> +run-if = crashreporter
> +[browser_gmpplugin_crashinfobar.js]

The conditions go on the line after the test file name (see e.g. browser_pluginCrashCommentAndURL.js below).

::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
@@ +31,5 @@
> +
> +  // Clear data in CrashManager in case previous tests caused something
> +  // to be added.
> +  Task.spawn(function* () {
> +    let store = yield Services.crashmanager._getStore();

This is quite likely to run into concurrency issues (the spawned task here runs async, so we don't have any guarantees on when this will finish).
Once we task-ify the test functions, we can just do this directly without the Task.spawn() and end up with defined behaviour.

I would move this up to before we start to init things for this test.

@@ +44,5 @@
> +  let tab = null;
> +  // This sucks. We don't know when the registration will finish on the GMP
> +  // thread, but we need to wait for it. Do something better... have
> +  // addPluginDirectory return a promise, maybe?
> +  setTimeout(function start() {

This can already use promises, e.g. something like this if this was task-ified:
> yield new Promise((resolve, reject) => setTimeout(resolve, 500));

I am optimistic that we can get bug 1087246 fixed relatively quickly, at which point we can just easily switch to waiting that promise.

@@ +99,5 @@
> +      ok(!("Throttleable" in submitted),
> +        "Submit request should not be Throttleable.");
> +      is(submitted.ProcessType, "plugin", "Should specify ProcessType=plugin.");
> +
> +      // Cleanup

If anything throws/fails before getting here, we skip a lot of relevant cleanup.
Keep in mind that for mochitests, multiple tests are run in the same session/environment and test-failures here with incomplete cleanup may break subsequent tests.

Note that you can register multiple cleanup functions, so one approach would be to just add cleanup functions directly after changes that affect the environment.

@@ +170,5 @@
> +
> +function generateCrashEvent() {
> +  // Again we have to wait for fake plugin to launch before we crash it.
> +  // "executeSoon()" won't do the trick.
> +  setTimeout(() => {

We shouldn't need this timeout, see the comment in the test HTML file.

@@ +201,5 @@
> +      is(buttons[1].label, "Submit a crash report",
> +        "Correct label for button #2.");
> +    }
> +
> +    // Check FHR report submission

This part reports crashes (which go to Socorro / crash-stats.mozilla.org).
"Check crash report submission"?

::: browser/base/content/test/plugins/plugin_fake_h264.html
@@ +98,5 @@
> +      }
> +
> +      // pc1.setRemote finished, media should be running!
> +      function step6() {
> +        document.getElementById("log").innerHTML = "HIP HIP HOORAY";

Talking with jesup in IRC, the plugin is definitely instantiated at this point, so we just have to figure out the best way to have the main test code wait for this.

Comment 15

[Qeole]

Attachment: gmp_crashinfobar.patch (v3)

That's feedback indeed! :) Thank you Georg.

I've edited my patch to take all your comments (in comment #14) into account.
I'm not really familiar with tasks and promises, and I just hope there are not too many horrible things (the deferred promise for example?) in this version.
Note that the test still passes on my computer.

Comment 16

[Qeole]

Attachment: gmp_crashinfobar.patch (v3)

Whoops. Forgot to remove “waitForExplicitFinish();”. Updating patch.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8516436 [details] [diff] [review]
gmp_crashinfobar.patch (v3)

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

Thanks, that looks much cleaner!
Feedback below - no major issues, mostly just improvements.

::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
@@ +8,5 @@
> +const gTestRoot =
> +  rootDir.replace("chrome://mochitests/content/", "http://mochi.test:8888/");
> +
> +let gTestBrowser = null;
> +let gObserverDeferred = Promise.defer();

We should not use defer() anymore in new code, use |new Promise((resolve, reject) => ...)| instead:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Deferred
Also, i don't think we need a global for this.

@@ +46,5 @@
> +  setUpReportObserver();
> +
> +  // Open tab, set listeners
> +  let tab = gBrowser.loadOneTab(gTestRoot + "plugin_fake_h264.html",
> +    { inBackground: false });

I want us to use bug 1087246 before landing anyway, but you may want to wait for the plugin registration before loading the page, otherwise the plugin may not be avaible yet, causing the WebRTC set-up to fail.

@@ +48,5 @@
> +  // Open tab, set listeners
> +  let tab = gBrowser.loadOneTab(gTestRoot + "plugin_fake_h264.html",
> +    { inBackground: false });
> +  gTestBrowser = gBrowser.getBrowserForTab(tab);
> +  gTestBrowser.addEventListener("load", generateCrashEvent, true);

Lets use promiseTabLoaded(). We probably have to add it to plugins/head.js here (see browser/base/content/test/general/head.js).
Then we can fold generateCrashEvent() into this.

@@ +59,5 @@
> +
> +  let notification = null;
> +  let notificationPromise = waitForNotificationBar("plugin-crashed",
> +    gTestBrowser, (aNotification) => { notification = aNotification; });
> +  yield notificationPromise;

The promise is resolved with the notification bar, so this can be written as:
let notification = yield waitForNotificationBar(...)

@@ +74,5 @@
> +
> +  // Check infobar / buttons labels (iff locale is en-US)
> +  let chrome = Cc["@mozilla.org/chrome/chrome-registry;1"]
> +               .getService(Ci.nsIXULChromeRegistry);
> +  if (chrome.getSelectedLocale("global") == "en-US") {

Not really important, but i'd rather factor the locale detection out from here and set a flag where we initialize most things or in a defineLazyGetter().

@@ +99,5 @@
> +
> +  const SERVER_URL = "http://example.com/" +
> +    "browser/toolkit/crashreporter/test/browser/crashreport.sjs";
> +
> +  let testObserver = {

Maybe we can move this to something like:
function promiseCheckCrashSubmission() {
  return new Promise((resolve, reject) => {
    let testTestObserver = ...
    // ... register observer etc.
  };
}

@@ +159,5 @@
> +                      "Submission should be successful." );
> +
> +        store.reset();
> +
> +        gObserverDeferred.resolve();

If anything in this Task rejects or throws, we will wait on that deferred until things time out.
One way to solve that would be something like:
let task = Task.spawn(...);
task.then(resolve, reject);

@@ +201,5 @@
> +  let gmpRegistrationPromise = new Promise((resolve, reject) =>
> +    setTimeout(resolve, 500));
> +
> +  gmpRegistrationPromise.then(() => {
> +    Services.prefs.setBoolPref("media.gmp.plugin.crash", true);

The WebRTC setup in the content page is async. To have a reliably working test we have to make sure that we reached test6() in the content.

As the browser and content js are separate, we can listen to an event from the content, e.g. like so:
http://hg.mozilla.org/mozilla-central/annotate/5dde8ea48fef/browser/base/content/content.js#l310
... and fire it from the content step6() like this:
http://hg.mozilla.org/mozilla-central/annotate/5dde8ea48fef/browser/base/content/abouthome/aboutHome.js#l323

To make matters a little more complicated, we have e10s [1] coming up (running content in separate processes), so we need to start being aware of that in tests as well.
That means we have privileged code running in both processes, the main chrome script and (in the content process) the "framescript", which communicate via the message manager [2].

So, we need to
* inject a frame script that listens/waits for step6() to fire an event
* have the frame script send a message via the message manager
* have the test script listen to that message and then trigger the crash

This here is an example of a test-specific solution of injecting a frame script:
http://hg.mozilla.org/mozilla-central/annotate/5dde8ea48fef/browser/base/content/test/general/browser_restore_isAppTab.js#l64
... and this is the main chrome script adding/removing the listener for the message the frame script sends:
http://hg.mozilla.org/mozilla-central/annotate/5dde8ea48fef/browser/base/content/test/general/browser_restore_isAppTab.js#l73

[1] https://wiki.mozilla.org/Electrolysis
[2] https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/The_message_manager

::: browser/base/content/test/plugins/plugin_fake_h264.html
@@ +98,5 @@
> +      }
> +
> +      // pc1.setRemote finished, media should be running!
> +      function step6() {
> +        document.getElementById("log").innerHTML = "HIP HIP HOORAY";

Lets change this to something more descriptive.

Comment 18

[Qeole]

Attachment: gmp_crashinfobar.patch (v4)

Thanks again! I've made a new version of the patch. Fixes should respond to all of your comments (in comment #17).

Also added a |return;| in main task if notification does not appear, since following checks fail (with “notification is null” error) and test freezes until timeout is reached in this case.

I tried to also replace the |Promise.defer()| from promises I copied into file head.js. Seems to work, but you'd better check I did it correctly.

Comment 19

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8517851 [details] [diff] [review]
gmp_crashinfobar.patch (v4)

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

Great, that looks good.
I'll see that i get to the blocking bug this week, unless someone else can get there.
Would you happen to be interested in that bug too or are you too short on time?

::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
@@ +16,5 @@
> +
> +// Tests that gmp plugin crash submissions work properly
> +
> +add_task(function* () {
> +  // Clear data in CrashManager in case previous tests caused sth to be added

s/sth/something/ ?

@@ +25,5 @@
> +  let chrome = Cc["@mozilla.org/chrome/chrome-registry;1"]
> +               .getService(Ci.nsIXULChromeRegistry);
> +  let localeIsEnUS = (chrome.getSelectedLocale("global") == "en-US");
> +
> +  // Setup

Slight preference for moving all the setup code to a separate |add_task(function* setup() ...);|

@@ +45,5 @@
> +    gmp.removePluginDirectory(pluginChannel.file.path)
> +  });
> +
> +  // Set up observer for submission report (and return a promise)
> +  let promiseObserverEnds = setUpReportObserver();

I don't think we need this here yet.
Let's move this down to right where we use it.

@@ +121,5 @@
> +        Assert.equal( data, "success",
> +                      "Report should have been submitted successfully." );
> +        Assert.equal(topic, "crash-report-status", "Topic is correct.");
> +        Assert.ok( subject instanceof Ci.nsIPropertyBag2,
> +                   "Subject should be a property bag." );

To reduce nesting & improve on the linearity of the test code here, i'd prefer us to resolve with |subject| here and do the actual checks etc. in the main test function.
Then we can just call this |promiseCrashSubmittedNotification| or something similar.

@@ +217,5 @@
> +/**
> + * Frame script that will be injected into the test browser to notify main
> + * script when media is loaded and running.
> + */
> +function frameScript() {

I'd rather see the helper functions on top, before the actual test functions and task.

::: browser/base/content/test/plugins/head.js
@@ +241,5 @@
> +      tab.linkedBrowser.loadURI(url);
> +  });
> +}
> +
> +function whenTabLoaded(aTab, aCallback) {

I don't think we need that function here.

::: browser/base/content/test/plugins/plugin_fake_h264.html
@@ +99,5 @@
> +
> +      // pc1.setRemote finished, media should be running!
> +      function step6() {
> +        document.getElementById("log").innerHTML =
> +          "Fake GMP plugin instanciated, media should be running...";

Nit: "instantiated".

Comment 20

[Qeole]

(In reply to Georg Fritzsche [:gfritzsche] from comment #19)

> Great, that looks good.
> I'll see that i get to the blocking bug this week, unless someone else can
> get there.
> Would you happen to be interested in that bug too or are you too short on
> time?

Yes I am interested :)! *And* short of time, actually. But I think I can find some time as I did lately, so it should be alright as long as there is nothing urgent.
 
> @@ +45,5 @@
> > +    gmp.removePluginDirectory(pluginChannel.file.path)
> > +  });
> > +
> > +  // Set up observer for submission report (and return a promise)
> > +  let promiseObserverEnds = setUpReportObserver();
> 
> I don't think we need this here yet.
> Let's move this down to right where we use it.

I'll have to check this. By the time I wrote it, it was (at first) called later, but then the observer wouldn't work, and I pulled it up on purpose. Specifically, I think it wouldn't work if setting up the observer was performed after crashing the plugin (even if it was before clicking on “Submit a report” button). I'm gonna check again how far I can put it in the main task.
 
> ::: browser/base/content/test/plugins/head.js
> @@ +241,5 @@
> > +      tab.linkedBrowser.loadURI(url);
> > +  });
> > +}
> > +
> > +function whenTabLoaded(aTab, aCallback) {
> 
> I don't think we need that function here.

It's called by function promiseTabLoaded(). Would you like it to be refactored?

Thanks for the other parts, I'll correct it and post a new patch.

Comment 21

[Georg Fritzsche [:gfritzsche]]

(In reply to Qeole [:qeole] from comment #20)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #19)
> 
> > Great, that looks good.
> > I'll see that i get to the blocking bug this week, unless someone else can
> > get there.
> > Would you happen to be interested in that bug too or are you too short on
> > time?
> 
> Yes I am interested :)! *And* short of time, actually. But I think I can
> find some time as I did lately, so it should be alright as long as there is
> nothing urgent.

Ah, that would be great.

>  
> > @@ +45,5 @@
> > > +    gmp.removePluginDirectory(pluginChannel.file.path)
> > > +  });
> > > +
> > > +  // Set up observer for submission report (and return a promise)
> > > +  let promiseObserverEnds = setUpReportObserver();
> > 
> > I don't think we need this here yet.
> > Let's move this down to right where we use it.
> 
> I'll have to check this. By the time I wrote it, it was (at first) called
> later, but then the observer wouldn't work, and I pulled it up on purpose.
> Specifically, I think it wouldn't work if setting up the observer was
> performed after crashing the plugin (even if it was before clicking on
> “Submit a report” button). I'm gonna check again how far I can put it in the
> main task.

I think you only need to set the observer up right before triggering the crash. If you need to set it up earlier, something strange is going on :)

> > ::: browser/base/content/test/plugins/head.js
> > @@ +241,5 @@
> > > +      tab.linkedBrowser.loadURI(url);
> > > +  });
> > > +}
> > > +
> > > +function whenTabLoaded(aTab, aCallback) {
> > 
> > I don't think we need that function here.
> 
> It's called by function promiseTabLoaded(). Would you like it to be
> refactored?

It is only called by promiseTabLoaded(), so i think we should just use the code there directly (we want to use less callbacks anyway over promises etc.).

Comment 22

[Qeole]

Attachment: gmp_crashinfobar.patch (v5)

(In reply to Georg Fritzsche [:gfritzsche] from comment #19)
> @@ +25,5 @@
> > +  let chrome = Cc["@mozilla.org/chrome/chrome-registry;1"]
> > +               .getService(Ci.nsIXULChromeRegistry);
> > +  let localeIsEnUS = (chrome.getSelectedLocale("global") == "en-US");
> > +
> > +  // Setup
> 
> Slight preference for moving all the setup code to a separate
> |add_task(function* setup() ...);|

Eventually I split the single task into three ones (setup / loading tab and preparing crash / verifications).

> @@ +45,5 @@
> > +    gmp.removePluginDirectory(pluginChannel.file.path)
> > +  });
> > +
> > +  // Set up observer for submission report (and return a promise)
> > +  let promiseObserverEnds = setUpReportObserver();
> 
> I don't think we need this here yet.
> Let's move this down to right where we use it.

Could move it down after all. But it must be executed before infobar appears. 

> 
> ::: browser/base/content/test/plugins/head.js
> @@ +241,5 @@
> > +      tab.linkedBrowser.loadURI(url);
> > +  });
> > +}
> > +
> > +function whenTabLoaded(aTab, aCallback) {
> 
> I don't think we need that function here.

I removed the two small functions and directly call |promiseTabLoadEvent()| (instead of |promiseTabLoaded()|).

Other issues from comment #19 are fixed as well. Test still passes on my computer.

Comment 23

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8521676 [details] [diff] [review]
gmp_crashinfobar.patch (v5)

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

::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
@@ +6,5 @@
> +
> +// Tests that gmp plugin crash submissions work properly
> +
> +let gTestBrowser = null;
> +let rootDir = getRootDirectory(gTestPath);

const gRootDir

@@ +9,5 @@
> +let gTestBrowser = null;
> +let rootDir = getRootDirectory(gTestPath);
> +const gTestRoot =
> +  rootDir.replace("chrome://mochitests/content/", "http://mochi.test:8888/");
> +const gSERVER_URL = "http://example.com/" +

Lets stay with one casing style.

@@ +58,5 @@
> +    let testObserver = {
> +      observe: function(subject, topic, data) {
> +        if (data == "submitting") // not done yet
> +          return;
> +        resolve({"subject": subject, "topic": topic, "data": data});

No need for string keys here, just do |{subject: subject, ...|

@@ +61,5 @@
> +          return;
> +        resolve({"subject": subject, "topic": topic, "data": data});
> +      },
> +
> +      QueryInterface: function(iid) {

You can use XPCOMUtils.generateQI() here.

@@ +82,5 @@
> +    crashReporter.serverURL = NetUtil.newURI(gSERVER_URL);
> +
> +    let os = Cc["@mozilla.org/observer-service;1"]
> +             .getService(Ci.nsIObserverService);
> +    os.addObserver(testObserver, "crash-report-status", true);

Just use Services.obs.addObserver

@@ +142,5 @@
> +});
> +
> +add_task(function* check() {
> +  // Set up observer for submission report (and return a promise)
> +  let promiseCrashSubmittedNotification = setUpReportObserver();

We should set this up before we are launching what will end up triggering the crash.
As is, this may run into timing issues - the setup from triggering the tab load to here is synchronous, but we don't know if there will be events processed etc. between loadAndCrash() and check().

@@ +212,5 @@
> +  let file = Services.dirsvc.get("UAppData", Ci.nsILocalFile);
> +  file.append("Crash Reports");
> +  file.append("submitted");
> +  file.append(remoteID + ".txt");
> +  file.remove(false);

This and the |store.reset()| would be nice to have in a registerCleanupFunction() in case of failures.
Note that registerCleanupFunction() handles promises, so you can either return one or pass a task to it.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Note that this looks pretty clean now, the above is just the more detailed feedback at this stage.

Comment 25

[Georg Fritzsche [:gfritzsche]]

Felipe, could you maybe help out here while i'm on vacation?
This should be nearly done and ready for review when the blocking bug is resolved.

Comment 26

[Qeole]

Attachment: gmp_crashinfobar.patch (v6)

(In reply to Georg Fritzsche [away Nov 15 - Nov 30] [:gfritzsche] from comment #23)

> @@ +142,5 @@
> > +});
> > +
> > +add_task(function* check() {
> > +  // Set up observer for submission report (and return a promise)
> > +  let promiseCrashSubmittedNotification = setUpReportObserver();
> 
> We should set this up before we are launching what will end up triggering
> the crash.
> As is, this may run into timing issues - the setup from triggering the tab
> load to here is synchronous, but we don't know if there will be events
> processed etc. between loadAndCrash() and check().

I merged the two tasks.

> @@ +212,5 @@
> > +  let file = Services.dirsvc.get("UAppData", Ci.nsILocalFile);
> > +  file.append("Crash Reports");
> > +  file.append("submitted");
> > +  file.append(remoteID + ".txt");
> > +  file.remove(false);
> 
> This and the |store.reset()| would be nice to have in a
> registerCleanupFunction() in case of failures.
> Note that registerCleanupFunction() handles promises, so you can either
> return one or pass a task to it.

I've added the |registerCleanupFunction()| calls, but I'm afraid I can't see here what I should use promises/tasks for? Could you please be more explicit on this point?

Comment 27

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8522221 [details] [diff] [review]
gmp_crashinfobar.patch (v6)

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

So, this should be the last time for me being pedantic :)
Next we should wait for the blocking bug to be resolved, get a final review and check that it's ok on try.

::: browser/base/content/test/plugins/browser_gmpplugin_crashinfobar.js
@@ +200,5 @@
> +                "Should specify ProcessType=plugin." );
> +
> +  // Clean up
> +  // First remove our fake submitted report
> +  registerCleanupFunction(() => {

We should move this up to where we first know that we submitted a crash report.
I would also include store.reset() there already.

Comment 28

[Georg Fritzsche [:gfritzsche]]

(In reply to Qeole [:qeole] from comment #26)
> I've added the |registerCleanupFunction()| calls, but I'm afraid I can't see
> here what I should use promises/tasks for? Could you please be more explicit
> on this point?

That was just in case you need to wait on something async (like |yield cm._getStore()|) - but if you don't need it how you do it that's fine too.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Felipe can help finishing this up next week or so.

Comment 30

[Qeole]

Attachment: gmp_crashinfobar.patch (v7)

Editied patch with minor modification in response to comment #27.

(In reply to Georg Fritzsche [away Nov 15 - Nov 30] [:gfritzsche] from comment #28)
> > I've added the |registerCleanupFunction()| calls, but I'm afraid I can't see
> > here what I should use promises/tasks for? Could you please be more explicit
> > on this point?
> 
> That was just in case you need to wait on something async (like |yield
> cm._getStore()|) - but if you don't need it how you do it that's fine too.

OK. I did not use it, as |yield cm._getStore()| also needs to be run inside the main task (for checks on crash manager), so I called it just before the |registerCleanupFunction()|.

Not flagging for feedback here, I'll now focus on blocking bug (bug 1087246).

Comment 32

[Maire Reavy [:mreavy]]

Hi qeole - We just found a regression in GMP crash reporting that we have a fix for.  (See Bug 1145432 for details.) This highlights that we don't have test coverage for GMP crash reporting (which the patch on this bug should give us).  Do you think we can get a patch up for review soon?

Hi spohl -- Do you think you'd be able to help get this patch up for review?  gfritzsche mentioned that you might be able to help.  gfritzsche is swamped with other work, and I'd feel much more comfortable if we had test coverage for this -- especially with EME (which uses GMP) getting ready to ship in Fx38.

Thanks!

Comment 33

[Benjamin Smedberg]

spohl needs to focus on update orphaning and this is well outside his scope of install/update work.

Comment 34

[Qeole]

Hi mreavy,
I saw that on #media on IRC before the server I use got powered off (it should be up again soon).

We're nearly done here with bug 1046052. What's missing is actually the promise for mozIGeckoMediaPluginService.(add|remove)PluginDirectory, which I tried to implement in bug 1087246; but I have an issue there and can't make it work without crashing firefox − haven't found where it comes from yet.

Sadly I don't have so much time to work on this for now; I'll try to look at it again, but I can't be sure to solve it soon enough for Fx38 on my own.

Comment 35

[Georg Fritzsche [:gfritzsche]]

mreavy, i think this bug is nearly finished but pending bug 1087246 - if someone could clear up the issue there, this bug should be cleared up quickly.

Comment 36

[Qeole]

Attachment: gmp_crashinfobar.patch (v8)

Here is a new version of patch with several points to consider:

1) It's been rebased on current version of mozilla-central.

2) v7 of this patch was from November. When I tried to run it again this week I met an issue I did not face at that time: the "MediaRunning" event dispatched by the content code (plugin_fake_h264.html file) is sent before adequate listener is set up in the frame script created from chrome code (frameScript() function).
So I used a setInterval() to repeat dispatching of the event (if someone has a cleaner solution, I'm interested).

3) Bug 1087246 advances well, but I also realized that the test could run and pass the same way without calling |addPluginDirectory()| at all (commented line 108 to 117 in file browser_gmpplugin_crashinfobar.js in patch). Actually, I believe this calls always fail to properly register the fake plugin (but I'm not 100% sure about that). Could we just do without this call? Or is there a good reason to call it? I need the opinion of someone who knows better than I do about it.

Asking for review to Georg, but I am not sure he has enough time to do this.

Comment 37

[Georg Fritzsche [:gfritzsche]]

(In reply to Qeole [:qeole] from comment #36)
> 2) v7 of this patch was from November. When I tried to run it again this
> week I met an issue I did not face at that time: the "MediaRunning" event
> dispatched by the content code (plugin_fake_h264.html file) is sent before
> adequate listener is set up in the frame script created from chrome code
> (frameScript() function).
> So I used a setInterval() to repeat dispatching of the event (if someone has
> a cleaner solution, I'm interested).
> 
> 3) Bug 1087246 advances well, but I also realized that the test could run
> and pass the same way without calling |addPluginDirectory()| at all
> (commented line 108 to 117 in file browser_gmpplugin_crashinfobar.js in
> patch). Actually, I believe this calls always fail to properly register the
> fake plugin (but I'm not 100% sure about that). Could we just do without
> this call? Or is there a good reason to call it? I need the opinion of
> someone who knows better than I do about it.

(jesup is on PTO, so trying cpearce for open questions)
cpearce, do you know a proper way for 2) from other tests?
For 3), do you know if we always register the gmp plugin now somewhere? Can we potentially skip setting it up in the test here?

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #37)
> (In reply to Qeole [:qeole] from comment #36)
> > 2) v7 of this patch was from November. When I tried to run it again this
> > week I met an issue I did not face at that time: the "MediaRunning" event
> > dispatched by the content code (plugin_fake_h264.html file) is sent before
> > adequate listener is set up in the frame script created from chrome code
> > (frameScript() function).
> > So I used a setInterval() to repeat dispatching of the event (if someone has
> > a cleaner solution, I'm interested).
> > 
> > 3) Bug 1087246 advances well, but I also realized that the test could run
> > and pass the same way without calling |addPluginDirectory()| at all
> > (commented line 108 to 117 in file browser_gmpplugin_crashinfobar.js in
> > patch). Actually, I believe this calls always fail to properly register the
> > fake plugin (but I'm not 100% sure about that). Could we just do without
> > this call? Or is there a good reason to call it? I need the opinion of
> > someone who knows better than I do about it.
> 
> (jesup is on PTO, so trying cpearce for open questions)
> cpearce, do you know a proper way for 2) from other tests?

Sorry, I don't know anything about how to create the fake GMP from WebRTC code, or where to look to find tests that do that. ekr, abr, or someone else in #media might know.

> For 3), do you know if we always register the gmp plugin now somewhere? Can
> we potentially skip setting it up in the test here?

I believe the GMPProvider automatically registers EME plugins on startup now, I *think* that should happen everywhere we run the Firefox executable.

We also set MOZ_GMP_PATH in some test harnesses:

http://mxr.mozilla.org/mozilla-central/search?string=MOZ_GMP_PATH

I suspect since we now run the GMPProvider to add EME GMPs that this may not be necessary for EME mochitests at least. I haven't tested that theory.

It's not clear what you're trying to do here, but it may (or may not) help you to know that we fire the observer service notification "gmp-path-added" when mozIGeckoMediaPluginService.AddPluginDirectory() has succeeded.

Note: we need this test to cover EME plugins too, not just WebRTC's use of plugins. The gmp-clearkey plugins should be enabled and registered by default on all desktop platforms now.

You can create an EME GMP using this JS code:

navigator.requestMediaKeySystemAccess("org.w3.clearkey").then(
  function (access) {
    return access.createMediaKeys();
  }).then(
    function (mediaKeys) {
      console.log("created gmp-clearkey.");
    }
  );

When the "created gmp-clearkey" code runs, you've got a GMP that you can crash using the pref. The "mediaKeys" object corresponds to JS's link to the GMP. This should work in non-e10s on all desktop platforms.

Comment 39

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris Pearce (:cpearce) from comment #38)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #37)
> > (In reply to Qeole [:qeole] from comment #36)
> > > 2) v7 of this patch was from November. When I tried to run it again this
> > > week I met an issue I did not face at that time: the "MediaRunning" event
> > > dispatched by the content code (plugin_fake_h264.html file) is sent before
> > > adequate listener is set up in the frame script created from chrome code
> > > (frameScript() function).
> > > So I used a setInterval() to repeat dispatching of the event (if someone has
> > > a cleaner solution, I'm interested).
> > > 
> > > 3) Bug 1087246 advances well, but I also realized that the test could run
> > > and pass the same way without calling |addPluginDirectory()| at all
> > > (commented line 108 to 117 in file browser_gmpplugin_crashinfobar.js in
> > > patch). Actually, I believe this calls always fail to properly register the
> > > fake plugin (but I'm not 100% sure about that). Could we just do without
> > > this call? Or is there a good reason to call it? I need the opinion of
> > > someone who knows better than I do about it.
> > 
> > (jesup is on PTO, so trying cpearce for open questions)
> > cpearce, do you know a proper way for 2) from other tests?
> 
> Sorry, I don't know anything about how to create the fake GMP from WebRTC
> code, or where to look to find tests that do that. ekr, abr, or someone else
> in #media might know.

Forwarding... ekr?

Comment 40

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris Pearce (:cpearce) from comment #38)
> > For 3), do you know if we always register the gmp plugin now somewhere? Can
> > we potentially skip setting it up in the test here?
> 
> I believe the GMPProvider automatically registers EME plugins on startup
> now, I *think* that should happen everywhere we run the Firefox executable.
> 
> We also set MOZ_GMP_PATH in some test harnesses:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=MOZ_GMP_PATH
> 
> I suspect since we now run the GMPProvider to add EME GMPs that this may not
> be necessary for EME mochitests at least. I haven't tested that theory.

Ok, it looks like its always registered now, we can verify that on a try push.

> You can create an EME GMP using this JS code:
> 
> navigator.requestMediaKeySystemAccess("org.w3.clearkey").then(
>   function (access) {
>     return access.createMediaKeys();
>   }).then(
>     function (mediaKeys) {
>       console.log("created gmp-clearkey.");
>     }
>   );
> 
> When the "created gmp-clearkey" code runs, you've got a GMP that you can
> crash using the pref. The "mediaKeys" object corresponds to JS's link to the
> GMP. This should work in non-e10s on all desktop platforms.

qeole, you think you can add a section for that? Or should we move that to a separate patch?

Comment 41

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8584814 [details] [diff] [review]
gmp_crashinfobar.patch (v8)

Cancelling for now until we have comment 39 answered and incorporated :)

Comment 42

[Eric Rescorla (:ekr)]

I wrote some of the fake GMP code but Jesup added whatever code makes it crash.

Comment 43

[Qeole]

(In reply to Georg Fritzsche [:gfritzsche] from comment #40)
@Georg: I used the wrong flag anyway, I meant to ask for “feedback” and not “review”. So no problem at all for cancelling this.

> > You can create an EME GMP using this JS code:
> > 
> > navigator.requestMediaKeySystemAccess("org.w3.clearkey").then(
> >   function (access) {
> >     return access.createMediaKeys();
> >   }).then(
> >     function (mediaKeys) {
> >       console.log("created gmp-clearkey.");
> >     }
> >   );
> > 
> > When the "created gmp-clearkey" code runs, you've got a GMP that you can
> > crash using the pref. The "mediaKeys" object corresponds to JS's link to the
> > GMP. This should work in non-e10s on all desktop platforms.
> 
> qeole, you think you can add a section for that? Or should we move that to a
> separate patch?

I don't know, I didn't really understand what I'd be supposed to test with this code (and “EME” does not ring any bell to me). Could you please provide some details (in here or in IRC)?

Also meanwhile, should I start launching a try push to check whether current version of patch is passing?

Comment 44

[Mike Conley (:mconley) (:⚙️)]

I suspect my work in bug 1146955 is going to invalidate some of this work. Sorry about that.

Comment 45

[Qeole]

Not sure what the impact of bug 1146955 is.
I've been unable to work on this bug lately, plus attention for this bug seems to have dropped for now.

I'm unassigning, but you may ping me if it comes relevant again (and if someone has enough time to mentor me about it).

Comment 46

[Mark Banner (:standard8)]

Clearing NI for jesup, as this is either not relevant or maybe not necessary if this isn't being currently worked on.

Georg, is this still a suitable mentored bug?

Comment 47

[Andrew McCreight [:mccr8]]

gfritzsche has not been involved with Mozilla for more than a year so will unfortunately no longer be a suitable mentor.