Closed Bug 1273795 Opened 8 years ago Closed 8 years ago

Intermittent browser_net_service-worker-status.js | FATAL ERROR: AsyncShutdown timeout in ShutdownLeaks: Wait for cleanup to be finished before checking for leaks Conditions: [{"name":"DevTools: Wait until toolbox is destroyed","state":"(none)","filename"

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox50 fixed, firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: aryx, Assigned: jsnajdr)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1273795 - browser_net_service-worker-status.js: use ContentTask instead of CPOW
58 bytes, text/x-review-board-request
ochameau
: review+
Details 
I suspect this is caused by the assertions and errors described in bug 1293980.
Depends on: 1293980
Another possible why this test can fail, in addition to the securityInfo issue in bug 1293980, is that it resolves promises with a CPOW when registering and unregistering the service worker. See bug 1233497. I'm converting all CPOW calls with a ContentTask.

The test will start permafailing when you remove the "skipPermitUnload" parameter to removeTab at [1]. I'm going to do exactly this in another cleanup, where I start including a shared-head.js in netmonitor tests, and unify several duplicate implementations of removeTab into one.

[1] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/head.js#117
Assignee: nobody → jsnajdr
Blocks: 1269102
Comment on attachment 8780482 [details]
Bug 1273795 - browser_net_service-worker-status.js: use ContentTask instead of CPOW

https://reviewboard.mozilla.org/r/71182/#review69122

Thanks for this cleanup.

::: devtools/client/netmonitor/test/browser_net_service-worker-status.js:47
(Diff revision 1)
>    info("Performing requests...");
> -  debuggee.performRequests();
> +  yield ContentTask.spawn(tab.linkedBrowser, {}, function* () {
> +    content.wrappedJSObject.performRequests();
> +  });
> +
>    yield waitForNetworkEvents(monitor, REQUEST_DATA.length);

Couldn't that race?
It is a typical pattern where events may be fired while we wait for yield ContentTask.spawn.

To be less prone to race, you would do:
  let onEvents = waitForNetworkEvents();
  yield ContentTask.spawn(...performRequest...); // This yield looks optional
  yield onEvents;
Attachment #8780482 - Flags: review?(poirot.alex) → review+
Comment on attachment 8780482 [details]
Bug 1273795 - browser_net_service-worker-status.js: use ContentTask instead of CPOW

https://reviewboard.mozilla.org/r/71182/#review69122

> Couldn't that race?
> It is a typical pattern where events may be fired while we wait for yield ContentTask.spawn.
> 
> To be less prone to race, you would do:
>   let onEvents = waitForNetworkEvents();
>   yield ContentTask.spawn(...performRequest...); // This yield looks optional
>   yield onEvents;

Yes, it could race, thanks for pointing it out. Posted an updated patch with:
- use add_task and remove call to finish()
- waitForNetworkEvents() without a race
- yield in ContentTask.spawn(...performRequest...) can be useful for catching errors in the task
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a94448be42e2
browser_net_service-worker-status.js: use ContentTask instead of CPOW r=ochameau
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a94448be42e2
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: