Clean up pending protocol.js requests if connection aborts

RESOLVED FIXED in Firefox 37

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: jryans, Assigned: jryans)

Tracking

(Blocks 1 bug)

unspecified
Firefox 39
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

2.01 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
1.12 KB, patch
jryans
: review+
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
bgrins
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
Details
39 bytes, text/x-review-board-request
bgrins
: review+
Details
Assignee

Description

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

Updated

5 years ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Assignee

Comment 1

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

Pull down this commit:

hg pull review -r ee9205e75408b685218075c8781e14375bc67a83
Attachment #8557291 - Flags: review?(bgrinstead)
(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 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.
Attachment #8557291 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Blocks: 1117067
Assignee

Comment 5

4 years ago
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.
Assignee

Updated

4 years ago
Attachment #8557291 - Flags: review?(bgrinstead)
Assignee

Comment 6

4 years ago
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 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
Attachment #8557291 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Blocks: 1132505
Assignee

Updated

4 years ago
Blocks: 1134162
Assignee

Updated

4 years ago
Blocks: 1134166
Assignee

Comment 9

4 years ago
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
Attachment #8557291 - Flags: review?(jsantell)
Attachment #8557291 - Flags: review?(bgrinstead)
Assignee

Comment 10

4 years ago
Looking like all test issues resolved:

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

Thanks bgrins for extra help with Web Console tests!
Assignee

Comment 11

4 years ago
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 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).
Attachment #8557291 - Flags: review?(jsantell) → review+
Assignee

Comment 13

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

Blah, still more tests to fix.
Attachment #8557291 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Assignee

Comment 15

4 years ago
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
Attachment #8557291 - Flags: review+ → review?(bgrinstead)
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
Attachment #8557291 - Flags: review?(bgrinstead) → review+
Assignee

Comment 19

4 years ago
(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
Assignee

Comment 21

4 years ago
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
Attachment #8557291 - Flags: approval-mozilla-beta?
Attachment #8557291 - Flags: approval-mozilla-aurora?
Comment on attachment 8557291 [details]
MozReview Request: bz://1128027/jryans

It's early in Beta so taking uplift for both Beta/Aurora.
Attachment #8557291 - Flags: approval-mozilla-beta?
Attachment #8557291 - Flags: approval-mozilla-beta+
Attachment #8557291 - Flags: approval-mozilla-aurora?
Attachment #8557291 - Flags: approval-mozilla-aurora+
Patch 5 (the webaudio test changes) needs rebasing for beta.
Flags: needinfo?(jsantell)
Flags: needinfo?(bgrinstead)
Flags: in-testsuite+
Assignee

Updated

4 years ago
Flags: needinfo?(jsantell)
Flags: needinfo?(jryans)
Flags: needinfo?(bgrinstead)
Depends on: 1137966
Depends on: 1138576
Assignee

Comment 28

4 years ago
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
Attachment #8575349 - Flags: review?(bgrinstead)
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.
Attachment #8575349 - Flags: review?(bgrinstead) → review+
Assignee

Updated

4 years ago
Attachment #8575349 - Attachment description: 0006-Bug-1128027-Wait-for-inspector-link-in-Web-Console-t.patch → Wait for inspector link in Web Console test (beta only)
Is the fx-team version intended for Aurora38 as well?
Assignee

Comment 34

4 years ago
(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.
Assignee

Comment 35

4 years ago
Landed test cleanup change for fx-team:

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

Comment 37

4 years ago
Attachment #8557291 - Attachment is obsolete: true
Attachment #8619255 - Flags: review+
Attachment #8619256 - Flags: review+
Attachment #8619257 - Flags: review+
Attachment #8619258 - Flags: review+
Attachment #8619259 - Flags: review+
Attachment #8619260 - Flags: review+

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.