Closed Bug 1564105 Opened 5 years ago Closed 5 years ago

Enable WS Monitor in Nightly

Categories

(DevTools :: Netmonitor, task, P2)

Product:
DevTools
Component:
Netmonitor
Type:
task

Tracking

(firefox70 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: Honza, Assigned: tanhengyeow)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1564105 - Enable WS Monitor in Nightly. r=Honza
47 bytes, text/x-phabricator-request
Details | Review
ws-enable.diff
4.28 KB, patch
Details | Diff | Splinter Review

Let's enable WS monitor in Nightly since it's already usable and we are at the beginning of the cycle. This way we can get more testers and feedback.

Honza

This bug 1562839 can be used as an example of how to enable a feature only in Nightly

Changesets:
https://hg.mozilla.org/mozilla-central/rev/5289c222f0d0

Honza

Blocks: network-websocket-inspector
Type: enhancement → task
No longer depends on: network-websocket-inspector
Depends on: 1565499
No longer depends on: 1565499

Enable WS Monitor in Nightly

Assignee: nobody → E0032242
Status: NEW → ASSIGNED
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1a964205023
Enable WS Monitor in Nightly. r=Honza

Backed out for devtools failures e.g. /browser_toolbox_tool_remote_reopen.js

backout: https://hg.mozilla.org/integration/autoland/rev/920ebfd35011bedee14ea1c6138c7afcb19d3f91

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=devtools&revision=b1a964205023bdb3ef2f7cc11b912eca7da0c5a7

failure log 1: devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js | Front for server1.conn101.child1/webSocketActor18 still held in pool! - https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=256839570&repo=autoland&lineNumber=8399

[task 2019-07-17T01:43:08.230Z] 01:43:08 INFO - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js | Front for server1.conn101.child1/webSocketActor18 still held in pool! -
[task 2019-07-17T01:43:08.231Z] 01:43:08 INFO - Stack trace:
[task 2019-07-17T01:43:08.233Z] 01:43:08 INFO - chrome://mochikit/content/browser-test.js:test_ok:1576
[task 2019-07-17T01:43:08.235Z] 01:43:08 INFO - chrome://mochitests/content/browser/devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js:test/<:99
[task 2019-07-17T01:43:08.237Z] 01:43:08 INFO - chrome://mochitests/content/browser/devtools/client/framework/test/browser_toolbox_tool_remote_reopen.js:test:107
[task 2019-07-17T01:43:08.239Z] 01:43:08 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1383
[task 2019-07-17T01:43:08.241Z] 01:43:08 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1209
[task 2019-07-17T01:43:08.243Z] 01:43:08 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803

failure log 2: devtools/client/netmonitor/test/browser_net_pause.js | leaked 1 docShell(s) until shutdown https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=256836388&repo=autoland&lineNumber=10900

[task 2019-07-17T01:33:14.731Z] 01:33:14 INFO - TEST-INFO | Main app process: exit 0
[task 2019-07-17T01:33:14.732Z] 01:33:14 ERROR - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_pause.js | leaked 1 docShell(s) until shutdown
[task 2019-07-17T01:33:14.733Z] 01:33:14 INFO - TEST-INFO | devtools/client/netmonitor/test/browser_net_pause.js | docShell(s) leaked: [pid = 1205] [id = {ff186d21-556e-4133-bdf0-d1f0b4d004b6}]
[task 2019-07-17T01:33:14.737Z] 01:33:14 INFO - TEST-INFO | devtools/client/netmonitor/test/browser_net_pause.js | This test created 1 hidden window(s)
[task 2019-07-17T01:33:14.739Z] 01:33:14 INFO - TEST-INFO | devtools/client/netmonitor/test/browser_net_pause.js | This test created 1 hidden docshell(s)

Flags: needinfo?(E0032242)

The issue seems to come from the following code:

  /**
   * Close the WebSocketFront.
   *
   */
  destroy() {
    if (!this._client) {
      return null;
    }
    [...]
    return super.destroy();
  }

this._client is not set anywhere by the websocket front, so the destroy always bails out. I think this if (!this._client) is just a copy paste leftover and should be removed.

Flags: needinfo?(jdescottes)

Thanks Julian, appreciate it!

Hi Honza, I've updated the patch to reflect this :D

Flags: needinfo?(E0032242) → needinfo?(odvarko)

Thanks Julian!

I pushed the new patch to try, but I am still seeing a leak
https://treeherder.mozilla.org/#/jobs?repo=try&revision=262817ae513e8c059c612e666f9a0c8f959a1307&selectedJob=256924639

devtools/client/netmonitor/test/browser_net_pause.js | leaked 1 docShell(s) until shutdown

Any other tips?
Honza

Flags: needinfo?(odvarko) → needinfo?(jdescottes)

A general tip for leaks is to check if the destroy of your fronts and actors run to completion without errors (I simply add console logs usually).

Here the destroy of the actor crashes when calling webSocketEventService.removeListener.
First I saw that this.targetActor.window.windowUtils.currentInnerWindowID is not always the same between startListening and stopListening.
So I stored the id of the window in the actor instead of just storing a flag. But after that webSocketEventService.removeListener was still crashing, so I simply added a try catch around it :)
https://gist.github.com/juliandescottes/9dcd14021ffed248fab3e408745b9941

This failure reproduces in local debug builds, so you can take my patch as a starting point and figure out a cleaner way of addressing this, maybe with some help from platform folks?

Flags: needinfo?(jdescottes)
Attached patch ws-enable.diff — — Splinter Review

Thanks Julian for the analysis!

@hengyeow: I am attaching a patch that fixes the leaks for me (based on Julian's work), please push it to Phab and set me and Julian as reviewers, thanks.

Honza

Flags: needinfo?(E0032242)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c833d50284d
Enable WS Monitor in Nightly. r=Honza,jdescottes

Updated the patch!

Flags: needinfo?(E0032242)

https://hg.mozilla.org/mozilla-central/rev/8c833d50284d

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: