Closed Bug 916458 Opened 7 years ago Closed 7 years ago

Can't close browser when paused on a breakpoint

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox25 verified, firefox26 verified, firefox27 verified)

VERIFIED FIXED
Firefox 27
Tracking Status
firefox25 --- verified
firefox26 --- verified
firefox27 --- verified

People

(Reporter: vporof, Assigned: past)

Details

Attachments

(4 files, 1 obsolete file)

Attached file 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 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.
Needs a test, but is this what you had in mind Nick?
Attachment #805238 - Flags: review?(nfitzgerald)
Assignee: nobody → past
Status: NEW → ASSIGNED
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.
Attachment #805238 - Flags: review?(nfitzgerald) → review+
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.
Attachment #806130 - Flags: review?(nfitzgerald)
Attachment #805238 - Attachment is obsolete: true
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
Attachment #806130 - Flags: review?(nfitzgerald) → review+
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
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/4eb0dcb034a2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Attached file 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Lol, it's dumpn(e) throwing!
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!
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
This happens on Aurora too. Care to ask for approval Panos?
Flags: needinfo?(past)
Certainly, thanks for the reminder!
Flags: needinfo?(past)
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
Attachment #806130 - Flags: approval-mozilla-aurora?
Attached patch Patch for betaSplinter Review
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.
I don't think landing the test is necessary.
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
Attachment #816613 - Flags: approval-mozilla-beta?
Attachment #806130 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #816613 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: ioana.budnar
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.