791215 - Test plugin hang submission

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

Bug 791009 showed a minor mistake in the plugin hang submissions which should have been caught by an automated test. We have tests for crash submission, I think, but we should test hang submission and in particular the multiple-minidump bits.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Add missing hang submission test

Try: https://tbpl.mozilla.org/?tree=Try&rev=e888acccaf75

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix range error in fake crashreport server

Fix RangeError in fake crashreport server showing up in above try run:
> toolkit/crashreporter/test/browser/crashreport.sjs: RangeError: arguments array passed to Function.prototype.apply is too large on line 60

Comment 3

[Georg Fritzsche [:gfritzsche]]

And try run for that patch:
https://tbpl.mozilla.org/?tree=Try&rev=b458c7fe6639

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 661756 [details] [diff] [review]
Add missing hang submission test

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

::: dom/plugins/test/mochitest/test_hang_submit.xul
@@ +112,5 @@
> +    return;
> +  }
> +
> +  // Default plugin hang timeout is too high for mochitests
> +  Services.prefs.setIntPref("dom.ipc.plugins.timeoutSecs", 5);

Can you set this even lower without breaking things? 5 seconds is a long time to wait for a test.

@@ +125,5 @@
> +  // Override the crash reporter URL to send to our fake server
> +  Services.prefs.setCharPref("toolkit.crashreporter.pluginHangSubmitURL", SERVER_URL);
> +
> +  // Hook into plugin crash events
> +  Services.obs.addObserver(testObserver, "crash-report-status", true);  

nit: trailing whitespace on this line.

Comment 5

[Georg Fritzsche [:gfritzsche]]

> ::: dom/plugins/test/mochitest/test_hang_submit.xul
> > +  // Default plugin hang timeout is too high for mochitests
> > +  Services.prefs.setIntPref("dom.ipc.plugins.timeoutSecs", 5);
> 
> Can you set this even lower without breaking things? 5 seconds is a long
> time to wait for a test.

Good point, i just followed what the other hang-tests used so far. 

Chris, can you remember if you chose that value specifically in test_hanging.html? 
Did you run into issues with a too low value causing premature hang-kills on loaded machines?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It was definitely for test_hanging.  I don't recall what factored into its choice though, too long ago :).

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix range error in fake crashreport server

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Add missing hang submission test

Fine on try with 1s hang timeout:
https://tbpl.mozilla.org/?tree=Try&rev=0f074d007cb9

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/156f00b9ac92
https://hg.mozilla.org/integration/mozilla-inbound/rev/bafe7d48e4c5

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/156f00b9ac92
https://hg.mozilla.org/mozilla-central/rev/bafe7d48e4c5