Closed
Bug 1084979
Opened 10 years ago
Closed 10 years ago
test_ipc.html breaks subsequent tests that check for plugin/content crashes
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: poiru, Assigned: poiru)
References
Details
Attachments
(2 files, 1 obsolete file)
1.24 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
3.46 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
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()
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8507376 -
Flags: review?(jmaher)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Attachment #8507377 -
Flags: review?(jmaher)
Comment 3•10 years ago
|
||
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?
Attachment #8507376 -
Flags: review?(jmaher) → review-
Comment 4•10 years ago
|
||
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?
Attachment #8507377 -
Flags: review?(jmaher) → review-
Assignee | ||
Comment 5•10 years ago
|
||
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.
Attachment #8507376 -
Flags: review- → review?(jmaher)
Comment 6•10 years ago
|
||
See also bug 1010285.
Updated•10 years ago
|
Attachment #8507376 -
Flags: review?(jmaher) → review+
Comment 7•10 years ago
|
||
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.
Attachment #8507377 -
Flags: review- → review+
Assignee | ||
Comment 8•10 years ago
|
||
Apparently there was already a simpler fix for this in dom/media/tests/ipc/test_ipc.html. I updated the patch to use it.
Attachment #8507376 -
Attachment is obsolete: true
Attachment #8508858 -
Flags: review?(jmaher)
Comment 9•10 years ago
|
||
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
Attachment #8508858 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a779db2cfa58 https://hg.mozilla.org/mozilla-central/rev/506226a2e6c0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•