916458 - Can't close browser when paused on a breakpoint

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

Attachment: log.txt

STR:

1. Open http://htmlpad.org/debugger/
2. Open Debugger
3. Click me
4. Cmd+Q

Process hangs. The browser can't quit. Attached are the relevant bits of the protocol log.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 804892 [details]
log.txt

I think this is my fault, looks like if we get an error when getting the lastPausedUrl in an EventLoopStack, we should just shut down everything.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Ignore failures to get the lastPausedUrl when a tab is closing ().

Needs a test, but is this what you had in mind Nick?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 805238 [details] [diff] [review]
Ignore failures to get the lastPausedUrl when a tab is closing ().

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

Going to be tricky to test, not sure what the best way to do it is off the top of my head.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Ignore failures to get the lastPausedUrl when a tab is closing (with test)

Luckily the bug is reproducible even when abruptly closing a single window, and not just the whole browser, so I was able to write a test for it. I also slightly improved a few test helper functions, so you should give it another quick look.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 806130 [details] [diff] [review]
Ignore failures to get the lastPausedUrl when a tab is closing (with test)

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

In the future, please add 8 lines of context to your patches, it makes it a lot easier to see what is going on :)

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Sorry, bzexport doesn't seem to be as configurable as I thought it would be. Try run:

https://tbpl.mozilla.org/?tree=Try&rev=dd2c0acfe28c

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/4eb0dcb034a2

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4eb0dcb034a2

Comment 9

[Victor Porof [:vporof][:vp]]

Attachment: new logs.txt

This isn't fixed :(
With the exact same STR from comment 0, the exact same effect. However, there's a different error thrown now, seemingly unrelated.

Handler function BrowserTabList.prototype.onCloseWindow's delayed body threw an exception: TypeError: str.split is not a function

Comment 10

[Victor Porof [:vporof][:vp]]

Lol, it's dumpn(e) throwing!

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

This is actually a regression from bug 918073, but it only happens when devtools.debugger.log is enabled, which is why tbpl didn't catch it!

Comment 12

[Victor Porof [:vporof][:vp]]

This happens on Aurora too. Care to ask for approval Panos?

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Certainly, thanks for the reminder!

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 806130 [details] [diff] [review]
Ignore failures to get the lastPausedUrl when a tab is closing (with test)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 901712
User impact if declined: the browser will hang when closed with the debugger paused at a breakpoint
Testing completed (on m-c, etc.): it's been on m-c for a month now, without any ill effects
Risk to taking this patch (and alternatives if risky): low risk, developer-facing feature, the actual fix is about a dozen lines of code
String or IDL/UUID changes made by this patch: none

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch for beta

The beta channel doesn't have the debugger refactoring patches, so trying to uplift the test is futile. This is a patch with just the actual fix.

Comment 16

[Victor Porof [:vporof][:vp]]

I don't think landing the test is necessary.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 816613 [details] [diff] [review]
Patch for beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 901712
User impact if declined: the browser will hang when closed with the debugger paused at a breakpoint
Testing completed (on m-c, etc.): it's been on m-c for a month now, without any ill effects, and I tested a local build from mozilla-beta with this patch
Risk to taking this patch (and alternatives if risky): low risk, developer-facing feature, the actual fix is about a dozen lines of code
String or IDL/UUID changes made by this patch: none

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69175fe3bd87

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 816613 [details] [diff] [review]
Patch for beta

Any regressions should be isolated to dev tools, so we can take this regression fix.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/55180425e522

Comment 21

[Ioana (away)]

Verified as fixed on Firefox 25.0b9 (20131017174213), 26.0a2 (20131018004002) and 27.0a1 (20131018030206), on Windows 7, Ubuntu 13.04 and Mac OS X 10.8.4.