1238939 - browser_net_cached-status.js fails when run solo

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Alexandre Poirot [:ochameau]]

It may also fail for other net monitor tests...

It fails for two reasons:
#1 Reentrance issue during client close:
Handler function DebuggerClient.requester request callback threw an exception: InternalError: too much recursion
Stack: LocalDebuggerTransport.prototype.close@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:668:9
DebuggerClient.prototype.close/cleanup@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:369:7
DebuggerClient.prototype.close/detachClients@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:399:9
DebuggerClient.requester/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:294:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:93:14
emitOnObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:112:9
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:89:38
Request.prototype.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1218:29
DebuggerClient.prototype.onClosed/reject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1097:7
DebuggerClient.prototype.onClosed@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1104:5
LocalDebuggerTransport.prototype.close@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:666:9
DebuggerClient.prototype.close/cleanup@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:369:7

#2 Race during NetMonitorController cleanup:
console.error:
  Message: Error: TabTarget#getTrait() can only be called on remote tabs.
  Stack:
    TabTarget.prototype.getTrait@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/target.js:251:13
NetMonitorController.disconnect<@chrome://devtools/content/netmonitor/netmonitor-controller.js:273:9
NetMonitorController.shutdownNetMonitor<@chrome://devtools/content/netmonitor/netmonitor-controller.js:200:13

Comment 1

[Alexandre Poirot [:ochameau]]

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

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Fix NetMonitorController race on disconnect

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Fix reentrance loop with active requests during client close

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8706988 [details] [diff] [review]
Fix NetMonitorController race on disconnect

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

_connection.promise is already resolved when we reach this code in tests.
But this delay the execution of the rest of this `disconnect` method.
This ends up introducing the "TabTarget#getTrait() can only be called on remote tabs." exception.
Later in this `disconnect` method, we call target.getTrait(),
but target gets completely cleaned up because of this additional delay due to this promise.
`disconnect` is called on toolbox destroy, so the target is being destroyed at the end of toolbox destroy.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8706989 [details] [diff] [review]
Fix reentrance loop with active requests during client close

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

This reentrance issue introduces the infinite loop.

DebuggerClient.close gets called, which tries to call each client `detach` method:
  http://mxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.js#403
Doing that piles up a `detach` request, which I think is the active one,
which gets rejected over here:
  http://mxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.js#1098
And so loops back to `detachClients` inner function from DebuggerClient.close.

As in other places, first remove the about-to-be-processed requests before re(solv|jec)ing them.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8706989 [details] [diff] [review]
Fix reentrance loop with active requests during client close

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

It looks like this one introduces an intermittent, in dt1?
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e079fb9544&selectedJob=15341077
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":"resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js","lineNumber":2018,"stack":["resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:Toolbox.prototype.destroy/leakCheckObserver
Or is it a known intermittent?

It seems to always happen after devtools/client/framework/test/browser_toolbox_textbox_context_menu.js,
but I don't reproduce when running just this test. Is it just the last test of the chunk? :/

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8706989 [details] [diff] [review]
Fix reentrance loop with active requests during client close

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

Seems like some of this could be fixed by the client using async promises (bug 1233891)...  But there is still the event API which runs sync.

So, makes sense to me.

Comment 8

[Alexandre Poirot [:ochameau]]

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

Comment 9

[Alexandre Poirot [:ochameau]]

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

Comment 10

[Alexandre Poirot [:ochameau]]

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

Comment 11

[Alexandre Poirot [:ochameau]]

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

Comment 12

[Alexandre Poirot [:ochameau]]

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

Comment 13

[Alexandre Poirot [:ochameau]]

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

Comment 14

[Alexandre Poirot [:ochameau]]

It looks like the win7-debug-dt1 - AsyncShutdown timeout during browser_toolbox_textbox_context_menu.js intermittent is gone... At least that's what the last two try runs seems to say.

Comment 15

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/e3eba353a71e62dd3dacd28d67b038daf619a2d7
Bug 1238939 - Fix NetMonitorController race on disconnect. r=vporof

https://hg.mozilla.org/integration/fx-team/rev/88a1fe447c9e0407879b120a5d724c84af17c463
Bug 1238939 - Fix reentrance loop with active requests during client close. r=jryans

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3eba353a71e
https://hg.mozilla.org/mozilla-central/rev/88a1fe447c9e