1084979 - test_ipc.html breaks subsequent tests that check for plugin/content crashes

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[poiru]

For example:

$ ./mach mochitest-plain content/media/webspeech/synth/ipc/test/test_ipc.html dom/plugins/test/mochitest/test_CrashService_crash.html
...
163 INFO TEST-UNEXPECTED-ERROR | /tests/dom/plugins/test/mochitest/test_CrashService_hang.html | This test did not leave any crash dumps behind, but we were expecting some!
TEST-INFO
164 INFO TEST-UNEXPECTED-ERROR | /tests/dom/plugins/test/mochitest/test_CrashService_hang.html | This test left crash dumps behind, but we weren't expecting it to!
TEST-INFO
165 INFO TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_CrashService_hang.html | undefined assertion name - Result logged after SimpleTest.finish()
166 INFO TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_CrashService_hang.html | undefined assertion name - Result logged after SimpleTest.finish()

Comment 1

[poiru]

Attachment: Stop test_ipc.html from breaking subsequent tests that check for plugin/content crashes

Comment 2

[poiru]

Attachment: Use removeMessageListener instead of addMessageListener in SpecialPowers.unregisterProcessCrashObservers

I noticed this likely copy-paste mistake while debugging this issue. This is not related to the issue, but I'm too lazy to create a new bug just for this.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8507376 [details] [diff] [review]
Stop test_ipc.html from breaking subsequent tests that check for plugin/content crashes

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

this patch looks safe, but is this a problem on all platforms?  Is this intermittent?  I see this test pass and the whole job pass often.  Do we understand why we need to reregister?

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8507377 [details] [diff] [review]
Use removeMessageListener instead of addMessageListener in SpecialPowers.unregisterProcessCrashObservers

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

::: testing/specialpowers/content/specialpowers.js
@@ +70,5 @@
>    sendSyncMessage("SPProcessCrashService", { op: "register-observer" });
>  };
>  
>  SpecialPowers.prototype.unregisterProcessCrashObservers = function() {
> +  removeMessageListener("SPProcessCrashService", this._messageListener);

I need more context here, is this just completely wrong?

Comment 5

[poiru]

Comment on attachment 8507376 [details] [diff] [review]
Stop test_ipc.html from breaking subsequent tests that check for plugin/content crashes

(In reply to Joel Maher (:jmaher) from comment #3)
> Comment on attachment 8507376 [details] [diff] [review]
> Stop test_ipc.html from breaking subsequent tests that check for
> plugin/content crashes
> 
> Review of attachment 8507376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this patch looks safe, but is this a problem on all platforms?  Is this
> intermittent?  I see this test pass and the whole job pass often.  Do we
> understand why we need to reregister?

After moving content/ to dom/ (bug 946065), this issue causes various crash-related tests to permafail on try:
https://tbpl.mozilla.org/?tree=Try&rev=f60b7dc8e589

I checked which tests got reordered and tried to identify the one causing the permafails. I noticed that e.g. test_CrashService_crash.html consistently failed when it was run soon after any of the test_ipc.html tests. You can consistently reproduce the issue with the command in comment 0.

I don't know the exact cause of the issue, but this patch does indeed fix it. I suspect that the breakage is caused by the test_ipc.html tests messing with SpecialPowersObserver:
http://hg.mozilla.org/mozilla-central/annotate/33c0181c4a25/content/media/webspeech/synth/ipc/test/file_ipc.html#l79

(In reply to Joel Maher (:jmaher) from comment #4)
> Review of attachment 8507377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/specialpowers/content/specialpowers.js
> @@ +70,5 @@
> >    sendSyncMessage("SPProcessCrashService", { op: "register-observer" });
> >  };
> >  
> >  SpecialPowers.prototype.unregisterProcessCrashObservers = function() {
> > +  removeMessageListener("SPProcessCrashService", this._messageListener);
> 
> I need more context here, is this just completely wrong?

It certainly looks wrong. This is the original code:

  SpecialPowers.prototype.registerProcessCrashObservers = function() {
    addMessageListener("SPProcessCrashService", this._messageListener);
    sendSyncMessage("SPProcessCrashService", { op: "register-observer" });
  };

  SpecialPowers.prototype.unregisterProcessCrashObservers = function() {
    addMessageListener("SPProcessCrashService", this._messageListener);
    sendSyncMessage("SPProcessCrashService", { op: "unregister-observer" });
  };

It seems odd that both registerProcessCrashObservers and unregisterProcessCrashObservers call addMessageListener. I did not do any further investigation because this potential mistake does not affect the test_ipc.html issue.

Comment 6

[Martijn Wargers (dead)]

See also bug 1010285.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8507377 [details] [diff] [review]
Use removeMessageListener instead of addMessageListener in SpecialPowers.unregisterProcessCrashObservers

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

thanks for explaining this in more detail.

Comment 8

[poiru]

Attachment: Prevent test_ipc.html from breaking subsequent tests that check for plugin/content crashes

Apparently there was already a simpler fix for this in dom/media/tests/ipc/test_ipc.html. I updated the patch to use it.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8508858 [details] [diff] [review]
Prevent test_ipc.html from breaking subsequent tests that check for plugin/content crashes

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

nice cleanup

Comment 10

[poiru]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a779db2cfa58
https://hg.mozilla.org/integration/mozilla-inbound/rev/506226a2e6c0

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a779db2cfa58
https://hg.mozilla.org/mozilla-central/rev/506226a2e6c0