Intermittent devtools/client/application/test/browser_application_panel_list-domain-workers.js | Uncaught exception - at resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:602 - Error: 'unregister

RESOLVED FIXED in Firefox 62

Status

defect
P5
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: intermittent-bug-filer, Assigned: jdescottes)

Tracking

({intermittent-failure, regression})

unspecified
Firefox 62

Firefox Tracking Flags

(firefox61 unaffected, firefox62 fixed)

Details

(Whiteboard: [retriggered][stockwell fixed:timing])

Attachments

(1 attachment)

Filed by: cbrindusan [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=177238992&repo=autoland

https://queue.taskcluster.net/v1/task/S20deXXmRqKgd4pI2PMiuA/runs/0/artifacts/public/logs/live_backing.log

https://treeherder.mozilla.org/logviewer.html#?job_id=177238992&repo=autoland&lineNumber=4238

[task 2018-05-07T10:18:26.263Z] 10:18:26     INFO - TEST-START | devtools/client/application/test/browser_application_panel_list-domain-workers.js
[task 2018-05-07T10:18:27.511Z] 10:18:27     INFO - TEST-INFO | started process screentopng
[task 2018-05-07T10:18:27.973Z] 10:18:27     INFO - TEST-INFO | screentopng: exit 0
[task 2018-05-07T10:18:27.974Z] 10:18:27     INFO - Buffered messages logged at 10:18:26
[task 2018-05-07T10:18:27.976Z] 10:18:27     INFO - Entering test bound 
[task 2018-05-07T10:18:27.977Z] 10:18:27     INFO - Adding a new tab with URL: http://example.com/browser/devtools/client/application/test/service-workers/simple.html
[task 2018-05-07T10:18:27.978Z] 10:18:27     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
[task 2018-05-07T10:18:27.978Z] 10:18:27     INFO - Tab added and finished loading
[task 2018-05-07T10:18:27.979Z] 10:18:27     INFO - Buffered messages logged at 10:18:27
[task 2018-05-07T10:18:27.980Z] 10:18:27     INFO - Wait until the service worker appears in the application panel
[task 2018-05-07T10:18:27.981Z] 10:18:27     INFO - TEST-PASS | devtools/client/application/test/browser_application_panel_list-domain-workers.js | First service worker registration is displayed for the correct domain - 
[task 2018-05-07T10:18:27.984Z] 10:18:27     INFO - Navigate to another page for a different domain with no service worker
[task 2018-05-07T10:18:27.985Z] 10:18:27     INFO - Waiting for event: 'navigate' on TabTarget:[object XULElement].
[task 2018-05-07T10:18:27.985Z] 10:18:27     INFO - Got event: 'navigate' on TabTarget:[object XULElement].
[task 2018-05-07T10:18:27.988Z] 10:18:27     INFO - TEST-PASS | devtools/client/application/test/browser_application_panel_list-domain-workers.js | No service workers are shown for an empty page in a different domain. - 
[task 2018-05-07T10:18:27.989Z] 10:18:27     INFO - Navigate to another page for a different domain with another service worker
[task 2018-05-07T10:18:27.993Z] 10:18:27     INFO - Waiting for event: 'navigate' on TabTarget:[object XULElement].
[task 2018-05-07T10:18:27.994Z] 10:18:27     INFO - Got event: 'navigate' on TabTarget:[object XULElement].
[task 2018-05-07T10:18:27.994Z] 10:18:27     INFO - Got event: 'navigate' on TabTarget:[object XULElement].
[task 2018-05-07T10:18:27.995Z] 10:18:27     INFO - Wait until the service worker appears in the application panel
[task 2018-05-07T10:18:27.999Z] 10:18:27     INFO - TEST-PASS | devtools/client/application/test/browser_application_panel_list-domain-workers.js | Second service worker registration is displayed for the correct domain - 
[task 2018-05-07T10:18:28.000Z] 10:18:27     INFO - Unregister all service workers
[task 2018-05-07T10:18:28.000Z] 10:18:27     INFO - Buffered messages finished
[task 2018-05-07T10:18:28.001Z] 10:18:28     INFO - TEST-UNEXPECTED-FAIL | devtools/client/application/test/browser_application_panel_list-domain-workers.js | Uncaught exception - at resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:602 - Error: 'unregister' request packet has no destination.
[task 2018-05-07T10:18:28.001Z] 10:18:28     INFO - Stack trace:
[task 2018-05-07T10:18:28.002Z] 10:18:28     INFO - request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:602:13
[task 2018-05-07T10:18:28.002Z] 10:18:28     INFO - unregisterAllWorkers@chrome://mochitests/content/browser/devtools/client/application/test/head.js:64:11
Summary: Intermittent Test-Verify devtools/client/application/test/browser_application_panel_list-domain-workers.js | Uncaught exception - at resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:602 - Error: 'unregister → Intermittent devtools/client/application/test/browser_application_panel_list-domain-workers.js | Uncaught exception - at resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:602 - Error: 'unregister
Hi, did some retriggers to find where this originated and found the following;

Worked on this range: http://tinyurl.com/yc8u56sa

Seems like this fail started from here: http://tinyurl.com/y9r7so22 where it was worked on devtools/client/application/test/browser_application_panel_list-domain-workers.js 

Julian, could you take a look at this? Thank you.
Flags: needinfo?(jdescottes)
Whiteboard: [retriggered]
Thanks for logging! 

The issue is most likely that we attempt to unregister workers that are not yet fully registered and don't have a registration actor. See the code branch at https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/devtools/shared/client/root-client.js#172-180

2 options here:
- force each test to wait until workers are registered before calling unregisterAllWorkers
- wait for all worker forms to have a valid registration actor before attempting to unregister

Second option should be cleaner but implies:
- modifying the waitUntil() helper to support async predicates (or creating a separate asyncWaitUntil)
- also implies that the service worker will finish registering even if the page is closed

I have try pushes with both approaches ongoing, will report and clear ni? when they are done.
Went for the second option, adding a new dedicated asyncWaitUntil (as expected, modifying waitUntil to support both sync and async makes some other tests fail spectacularly).

This seems to fix the issue: 
- with patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=356c1ab0f3333d8925da679eea35bf3296fa81f4&selectedJob=177794593 no failure in 70 runs of dt3
- without patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=78cc5b1a3f90f271a2ee773cb127afa0c5ed98d0&selectedJob=177815180 10 failures in 80 runs of dt3
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
Comment on attachment 8974627 [details]
Bug 1459605 - Wait for all workers to have registrationActors before unregistering;

https://reviewboard.mozilla.org/r/243010/#review248830

`+r` with a minor suggestion about the `info` messages. Thanks a lot!

::: devtools/client/application/test/head.js:69
(Diff revision 1)
> +    const allWorkersRegistered =
> +      workers.service.every(worker => !!worker.registrationActor);
> +    return allWorkersRegistered;
> +  });
>  
> -  for (let worker of service) {
> +  info("All workers have a valid registrationActor and can be unregistered");

Could we swap the two `info` messages so they better reflect what is going on? i.e. the first one informing of the check for the registration actor, and then have the second one with the unregister message.
Attachment #8974627 - Flags: review?(balbeza) → review+
(In reply to Belén [:ladybenko] from comment #7)
> Comment on attachment 8974627 [details]
> Bug 1459605 - Wait for all workers to have registrationActors before
> unregistering;
> 
> https://reviewboard.mozilla.org/r/243010/#review248830
> 
> `+r` with a minor suggestion about the `info` messages. Thanks a lot!

Thanks for the review!
> 
> ::: devtools/client/application/test/head.js:69
> (Diff revision 1)
> > +    const allWorkersRegistered =
> > +      workers.service.every(worker => !!worker.registrationActor);
> > +    return allWorkersRegistered;
> > +  });
> >  
> > -  for (let worker of service) {
> > +  info("All workers have a valid registrationActor and can be unregistered");
> 
> Could we swap the two `info` messages so they better reflect what is going
> on? i.e. the first one informing of the check for the registration actor,
> and then have the second one with the unregister message.

Good point, done.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/149fcd977c3d
Wait for all workers to have registrationActors before unregistering;r=ladybenko
https://hg.mozilla.org/mozilla-central/rev/149fcd977c3d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 62 → ---
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Whiteboard: [retriggered] → [retriggered][stockwell fixed:timing]
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.