Closed Bug 1348547 Opened 4 years ago Closed 4 years ago

Intermittent devtools/client/aboutdebugging/test/browser_service_workers_status.js | Service worker is currently running - Got Stopped, expected Running

Categories

(DevTools :: about:debugging, defect)

Product:
DevTools
Component:
about:debugging
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 unaffected, firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox53 --- unaffected
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: aryx, Assigned: gbrown)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed])

Attachments

(1 file)

call releaseCachedProcesses()
620 bytes, patch
gkrizsanits
: review+
Details | Diff | Splinter Review 
https://treeherder.mozilla.org/logviewer.html#?job_id=84722558&repo=autoland

13:30:11     INFO - Entering test bound 
13:30:11     INFO - opening about:debugging
13:30:11     INFO - Adding a new tab with URL: about:debugging#workers
13:30:11     INFO - Buffered messages logged at 13:30:06
13:30:11     INFO - Tab added and finished loading
13:30:11     INFO - Adding a new tab with URL: http://example.com/browser/devtools/client/aboutdebugging/test/service-workers/delay-sw.html
13:30:11     INFO - Buffered messages logged at 13:30:07
13:30:11     INFO - Tab added and finished loading
13:30:11     INFO - Make the test page notify us when the service worker sends a message.
13:30:11     INFO - Buffered messages logged at 13:30:11
13:30:11     INFO - TEST-PASS | devtools/client/aboutdebugging/test/browser_service_workers_status.js | Found the service worker in the list - 
13:30:11     INFO - Buffered messages finished
13:30:11     INFO - TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser_service_workers_status.js | Service worker is currently running - Got Stopped, expected Running
13:30:11     INFO - Stack trace:
13:30:11     INFO -     chrome://mochikit/content/browser-test.js:test_is:911
13:30:11     INFO -     chrome://mochitests/content/browser/devtools/client/aboutdebugging/test/browser_service_workers_status.js:null:44
13:30:11     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
13:30:11     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
13:30:11     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
another about:debugging failure from march 17th, possibly related to bug 1329836 ?
Whiteboard: [stockwell needswork]
believe this might be fixed by bug 1329836
(In reply to Joel Maher ( :jmaher) from comment #5)
> most likely related to https://bugzilla.mozilla.org/show_bug.cgi?id=1348112
> which I narrowed down for bug 1329836.

I am not so sure, since bug 1329836 started much earlier (though 1329836 did increase in frequency just before 03-18).
This test was updated around 03-16 - bug 1345932. But failures did not start with those changes.
Retriggers on comment 15 now clearly indicate that this started with https://hg.mozilla.org/integration/autoland/rev/5e664d9cc14d3331264be607ae27c650352066b6 / bug 1336398, which simply increased the process count.

:mrbkap -- mostly just making you aware, but wouldn't mind if you had a look at the failure here! (It is a frequent failure, so we need someone to fix or disable the test soon).
Blocks: 1336398
Flags: needinfo?(mrbkap)
That is very surprising. Service worker debugging doesn't work in e10s-multi and these tests attempt to force a single content process [1][2]. So bug 1336398 really should have little to no effect on these tests. That suggests a bug in our process selector.

[1] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/devtools/client/aboutdebugging/test/browser_service_workers_status.js#15
[2] http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/devtools/client/aboutdebugging/test/head.js#425,433
(In reply to Blake Kaplan (:mrbkap) from comment #17)
> That is very surprising. Service worker debugging doesn't work in e10s-multi
> and these tests attempt to force a single content process [1][2]. So bug
> 1336398 really should have little to no effect on these tests. That suggests
> a bug in our process selector.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 72fe012899a1b27d34838ab463ad1ae5b116d76b/devtools/client/aboutdebugging/test/
> browser_service_workers_status.js#15
> [2]
> http://searchfox.org/mozilla-central/rev/
> 72fe012899a1b27d34838ab463ad1ae5b116d76b/devtools/client/aboutdebugging/test/
> head.js#425,433

Maybe it's about the tables processes we keep alive... Have you tried calling Services.ppmm.releaseCachedProcesses(); if it fixes the problem?
See Also: → 1329836
See Also: → 1345932
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19)
> Maybe it's about the tables processes we keep alive... Have you tried
> calling Services.ppmm.releaseCachedProcesses(); if it fixes the problem?

Thanks! I'm trying https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebde66316c44e062561ef39beaf438bb6484299a ... mixture of positive and negative signs so far. I'll check on that again on Monday...
Flags: needinfo?(gbrown)
Following up on your comment 19, I found that calling releaseCachedProcesses() does indeed appear to avoid this failure, but I saw new failures in non-e10s mode, particularly leaks. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebde66316c44e062561ef39beaf438bb6484299a.

If I call releaseCachedProcesses() only if gMultiProcessBrowser, all is well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc19e2dd2dddfe0b913313fe58473b572b8eb7c3

This appears to also avoid the frequent intermittent leaks in aboutdebugging mochitests reported in bug 1329836! :)
Flags: needinfo?(gbrown)
Attachment #8854152 - Flags: review?(gkrizsanits)
(In reply to Geoff Brown [:gbrown] from comment #23)
> Following up on your comment 19, I found that calling
> releaseCachedProcesses() does indeed appear to avoid this failure, but I saw
> new failures in non-e10s mode, particularly leaks. 

I wonder if ContentParent::ReleaseCachedProcesses should avoid creating sBrowserContentParents if it doesn't already exist.
Flags: needinfo?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) from comment #24)
> (In reply to Geoff Brown [:gbrown] from comment #23)
> > Following up on your comment 19, I found that calling
> > releaseCachedProcesses() does indeed appear to avoid this failure, but I saw
> > new failures in non-e10s mode, particularly leaks. 
> 
> I wonder if ContentParent::ReleaseCachedProcesses should avoid creating
> sBrowserContentParents if it doesn't already exist.

Yeah... I have a patch for that for the in preallocated process manager work just haven't landed it yet :(
Comment on attachment 8854152 [details] [diff] [review]
call releaseCachedProcesses()

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

Could you please try instead of using gMultiProcessBrowser in the test file adding this changeset to ContentParent.cpp and calling Services.ppmm.releaseCachedProcesses() unconditionally in the test?

 /*static*/ void
 ContentParent::ReleaseCachedProcesses()
 {
+  if (!GetPoolSize(NS_LITERAL_STRING(DEFAULT_REMOTE_TYPE))) {
+    return;
+  }
+

If that works please land it that way (it should), in case it does not then land your version and I'll look into it in a followup.
Attachment #8854152 - Flags: review?(gkrizsanits) → review+
That does work - thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a6f6fd4ad02458c2a3287d0d676baabb3caab40
Assignee: nobody → gbrown
Whiteboard: [stockwell needswork] → [stockwell fixed]
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8379d9a545
ReleaseCachedProcesses() between aboutdebugging tests; r=gabor
https://hg.mozilla.org/mozilla-central/rev/8d8379d9a545
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Can we get a rebased patch for Aurora and an approval request please?
status-firefox54: unaffected → affected
Flags: needinfo?(gbrown)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #32)
> Can we get a rebased patch for Aurora and an approval request please?

We'll need bug 1345932 on aurora first. There are very few of these failures on aurora...worth the effort?
Depends on: 1345932
Flags: needinfo?(gbrown)
See Also: 1345932 → 
Only because they've been getting incorrectly starred as bug 1352586.
Duplicate of this bug: 1329836
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.