1128027 - Clean up pending protocol.js requests if connection aborts

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

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

If a connection aborts unexpectedly, protocol.js can be left with queued requests that it forgets to reject.

Comment 1

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

Attachment: MozReview Request: bz://1128027/jryans

/r/3197 - Bug 1128027 - Clean up protocol.js pools after connection close. r=bgrins

Pull down this commit:

hg pull review -r ee9205e75408b685218075c8781e14375bc67a83

Comment 2

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04692abef1e6

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] from comment #2)
> Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04692abef1e6

I was going to ask if you could think of a way to add a test for this functionality, but looks like one of the existing tests is having this happen.  Could you fix this test failure and then add a new one that just opens and aborts a connection, expecting the normal destroy process to happen?

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

https://reviewboard.mozilla.org/r/3195/#review2625

See Comment 3 about the test failures

::: toolkit/devtools/server/protocol.js
(Diff revision 1)
> +        events.once(this.conn, "closed", () => this.destroy());

Can we use events.once for all connections?  Or is there an advantage to using addOneTimeListener when it is available.

Comment 5

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

https://reviewboard.mozilla.org/r/3195/#review2773

> Can we use events.once for all connections?  Or is there an advantage to using addOneTimeListener when it is available.

There is no advantage.  The issue is that the client vs. server expose different event APIs. :(

Bug 1042642 aims to clean up the APIs.  I added a comment here referencing that bug.

Comment 6

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

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

/r/3197 - Bug 1128027 - Clean up protocol.js pools after connection close. r=bgrins

Pull down this commit:

hg pull review -r 64be6485895782c8de40dcd646a97deca238c90e

Comment 7

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

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

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

https://reviewboard.mozilla.org/r/3195/#review2797

Still seeing leaks on debug builds for the test:

TypeError: gPanelWindow is null: testEditorAdded@chrome://mochitests/content/browser/browser/devtools/styleeditor/test/browser_styleeditor_loading.js:24:7
TEST-UNEXPECTED-FAIL | browser/devtools/styleeditor/test/browser_styleeditor_nostyle.js | leaked 2 window(s) until shutdown [url = chrome://mochitests/content/browser/browser/devtools/styleeditor/test/nostyle.html]
TEST-UNEXPECTED-FAIL | browser/devtools/styleeditor/test/browser_styleeditor_nostyle.js | leaked 1 docShell(s) until shutdown

Comment 9

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

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

/r/3197 - Bug 1128027 - Clean up protocol.js pools after connection close. r=bgrins
/r/4295 - Bug 1128027 - Repair sourceeditor test after protocol.js cleanup change. r=bgrins
/r/4297 - Bug 1128027 - Inspector destroy error was holding document alive. r=bgrins
/r/4299 - Bug 1128027 - Record protocol.js request headers for debugging. r=bgrins
/r/4301 - Bug 1128027 - Settle Web Audio requests in tests before teardown. r=jsantell
/r/4303 - Bug 1128027 - Rework Console tests that click links. r=bgrins

Pull down these commits:

hg pull review -r 3c47ad3d0aa8b9947aca8d7e69fa52f6370157a6

Comment 10

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

Looking like all test issues resolved:

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

Thanks bgrins for extra help with Web Console tests!

Comment 11

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

https://reviewboard.mozilla.org/r/3197/#review3465

I changed the overall approach here from last time.  Now, there are no more events, so the design is simpler.  Instead, the client loops over the set of fronts in the pools it has after the connection is closed, calling `cleanup` on them.

I realized after the previous review that no fix is needed on the server side: it already performs a similar `cleanup` like this after connection close.

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

Looks good! A contributor ran into the automation panel not rendering before the test wrapped up causing some failures, so you should also check browser_wa_inspector-width.js as well, which has a similar fix (but your method is cleaner).

Comment 13

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

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

Blah, still more tests to fix.

Comment 14

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

Okay, try is actually good this time:

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

Comment 15

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

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

/r/3197 - Bug 1128027 - Clean up protocol.js pools after connection close. r=bgrins
/r/4295 - Bug 1128027 - Repair sourceeditor test after protocol.js cleanup change. r=bgrins
/r/4297 - Bug 1128027 - Inspector destroy error was holding document alive. r=bgrins
/r/4299 - Bug 1128027 - Record protocol.js request headers for debugging. r=bgrins
/r/4301 - Bug 1128027 - Settle Web Audio requests in tests before teardown. r=jsantell
/r/4303 - Bug 1128027 - Rework Console tests that click links. r=bgrins

Pull down these commits:

hg pull review -r bd2c3059ba3612185f7273ed78b081ab6996de03

Comment 16

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

https://reviewboard.mozilla.org/r/4301/#review3517

Carry over previous r+.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

https://reviewboard.mozilla.org/r/3195/#review3521

Nice work tracking down all of these issues, this will be helpful

::: toolkit/devtools/client/dbg-client.jsm
(Diff revision 4)
> +    // In case destruction does not happen in the usual way (because the

This comment could be clearer, explaining that these pools are protocol.js fronts that haven't been destroyed because of an unexpected exit

Comment 18

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/856dd8473a6c
remote:   https://hg.mozilla.org/integration/fx-team/rev/07fcafcdcf3f
remote:   https://hg.mozilla.org/integration/fx-team/rev/04f6c95bdc28
remote:   https://hg.mozilla.org/integration/fx-team/rev/be4ab9a8d00c
remote:   https://hg.mozilla.org/integration/fx-team/rev/0bdcde0e7111
remote:   https://hg.mozilla.org/integration/fx-team/rev/a4b03fd5c367

Comment 19

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

(In reply to Brian Grinstead [:bgrins] from comment #17)
> ::: toolkit/devtools/client/dbg-client.jsm
> (Diff revision 4)
> > +    // In case destruction does not happen in the usual way (because the
> 
> This comment could be clearer, explaining that these pools are protocol.js
> fronts that haven't been destroyed because of an unexpected exit

Okay, I included a longer version[1].  Hopefully it's better now!

[1]: https://hg.mozilla.org/integration/fx-team/rev/856dd8473a6c#l1.13

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/856dd8473a6c
https://hg.mozilla.org/mozilla-central/rev/07fcafcdcf3f
https://hg.mozilla.org/mozilla-central/rev/04f6c95bdc28
https://hg.mozilla.org/mozilla-central/rev/be4ab9a8d00c
https://hg.mozilla.org/mozilla-central/rev/0bdcde0e7111
https://hg.mozilla.org/mozilla-central/rev/a4b03fd5c367

Comment 21

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

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

Approval Request Comment
[Feature/regressing bug #]: Unclear where regression was introduced, but issue reproduces at least back to beta
[User impact if declined]: DevTools will be left on screen in a broken state if a device connection is aborted in WebIDE or e10s page is used in certain conditions
[Describe test coverage new/current, TreeHerder]: Landed on m-c, new tests added there
[Risks and why]: Low, only affects clean up of DevTools toolbox when it closes
[String/UUID change made/needed]: None

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

It's early in Beta so taking uplift for both Beta/Aurora.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Patch 5 (the webaudio test changes) needs rebasing for beta.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4a427d7dc86
https://hg.mozilla.org/releases/mozilla-aurora/rev/010880539033
https://hg.mozilla.org/releases/mozilla-aurora/rev/e988c79a74f5
https://hg.mozilla.org/releases/mozilla-aurora/rev/8955f76dc200
https://hg.mozilla.org/releases/mozilla-aurora/rev/fa0ec48ad174
https://hg.mozilla.org/releases/mozilla-aurora/rev/bb174842be5c

Comment 25

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

The Web Audio patch ended up being unneeded on beta:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/021aac3d7804
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/4c02cca13dbe
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/569e2110f0ff
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/b82653e56ec9
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/012e92feffe6

Comment 26

[Ryan VanderMeulen [:RyanVM]]

mochitest-dt failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=279112&repo=mozilla-beta

Comment 27

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

Backed out from beta:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/eed281422403
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/85ca6f646762
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/21120474140d
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/6f6ec57ce6b9
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/e49cd895e078

Will try again when I get back (with beta Try run).

Comment 28

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

Attachment: Wait for inspector link in Web Console test (beta only)

On beta, an additional fix was required for web console tests.

I'll likely also land this on fx-team too, it seems generally correct.

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

Comment 29

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

fx-team try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d9c45fc4532

Comment 30

[Brian Grinstead [:bgrins]]

Comment on attachment 8575349 [details] [diff] [review]
Wait for inspector link in Web Console test (beta only)

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

r+ for beta-only (assuming a green try push).

We will need a different patch for fx-team that simply removes the thisTestLeaksUncaughtRejectionsAndShouldBeFixed line, since the type of _linkedToInspector has changed to a boolean: https://dxr.mozilla.org/mozilla-central/search?q=_linkedToInspector&redirect=true.

Comment 31

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

Attachment: Remove browser_result_format_as_string.js leak whitelist (fx-team only)

Here's the fx-team only version.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=399af86993c7

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Is the fx-team version intended for Aurora38 as well?

Comment 33

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

Beta try looks good.  Relanded on beta with additional test fix:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/abce8eb1a75e
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/f21e8fa80469
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/fa9b1bfa9f0e
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/fbef1b8d36e0
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9fb666f03801
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/7e2e728297e6

Comment 34

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #32)
> Is the fx-team version intended for Aurora38 as well?

The additional fx-team patch (attachment 8575452 [details] [diff] [review]) is essentially a "code cleanliness" change: it removes a promise rejection whitelist statement from a test which no longer whitelists anyway (since the error message content changed due to the other patches in this bug).  It should have no impact on test outcome, it just removes dead code.

It should be fine to also land on Aurora, but I don't see a strong need to put it there either, unless someone feels it's simpler to follow that way.

Comment 35

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

Landed test cleanup change for fx-team:

https://hg.mozilla.org/integration/fx-team/rev/0767aa455e03

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0767aa455e03

Comment 38

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

Attachment: MozReview Request: Bug 1128027 - Repair sourceeditor test after protocol.js cleanup change. r=bgrins

Comment 39

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

Attachment: MozReview Request: Bug 1128027 - Inspector destroy error was holding document alive. r=bgrins

Comment 40

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

Attachment: MozReview Request: Bug 1128027 - Record protocol.js request headers for debugging. r=bgrins

Comment 41

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

Attachment: MozReview Request: Bug 1128027 - Settle Web Audio requests in tests before teardown. r=jsantell

Comment 42

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

Attachment: MozReview Request: Bug 1128027 - Rework Console tests that click links. r=bgrins

Comment 43

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

Attachment: MozReview Request: Bug 1128027 - Clean up protocol.js pools after connection close. r=bgrins