Closed Bug 1248303 Opened 8 years ago Closed 8 years ago

Pause on Exceptions does not pause after reopening devtools

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(firefox44 unaffected, firefox46+ fixed, firefox47+ verified, firefox48 verified)

VERIFIED FIXED
Firefox 48
Tracking Status
firefox44 --- unaffected
firefox46 + fixed
firefox47 + verified
firefox48 --- verified

People

(Reporter: bob+bmo, Assigned: jlong)

References

Details

(Keywords: regression)

Attachments

(2 files)

Test case: http://codepen.io/anon/pen/jWJExa

1. Open devtools on Debugger tab
2. Enable "Pause on Exceptions" in the Debugger options
3. Close devtools
4. Open devtools
5. Click the "Test Exceptions" button
6. Debugger should break on the uncaught exception

Expected results:

When devtools are opened to the Debugger panel, the debugger should break on the line that throws the exception.


Actual results:

Debugger does not break. Console reports "uncaught exception: ex". Notice that Pause on Exceptions is still ticked.


Affects:

Firefox Aurora 46.0a2 build 20160214004015
Firefox Nightly 47.0a1 build 20160214030236

Tested on Windows 7 SP1 x64, with clean Firefox profiles.


Works correctly on:

Firefox 44.0.1 build 20160205155049
Chrome 48.0.2564.109 m
Internet Explorer 11.0.9600.18204, update 11.0.28


Notes:

Pause on Exceptions works when first enabled (e.g. after step 2 above). It's only after closing and re-opening the devtools, or the browser as a whole, that it stops working.

Disabling and re-enabling Pause on Exceptions works, until the panel is closed.

A wild guess without having seen the source: perhaps the debugger exception handler is only attached when the option is first enabled, and not when the debugger is (re)opened?

This appears to be a regression from the last week or two. I first noticed it occurring on Aurora after an update within the last week, but I don't always keep it up to date. Possibly related to the new always-on debugger?
Possibly related to bug 1052072, however that is an older bug and this one does not affect Fx 44 (release) as far as I can tell.
Keywords: regression
[Tracking Requested - why for this release]: Regression breaks devtools feature and should not make its way into stable. Workaround exists but is not obvious and fairly inconvenient.
Assignee: nobody → jlong
Flags: needinfo?(jlong)
Recent regression, tracking. James once you have a viable fix please request uplift to aurora 46. Thanks!
This is a regression that breaks existing functionality. It should be fixed asap. Marking as P1.
Priority: -- → P1
Attached patch 1248303.patchSplinter Review
The way these flags work is really weird. We do not actually set the handler for this in the backend in the `reconfigure` request, it's only done in the `resume` request, and dbg-client only sends its internal state of the flags. All of this results in a confusing mess and I need to set these flags at a specific point in time.
Attachment #8726346 - Flags: review?(ejpbruel)
Eddy, we should try to get this in before the uplift next week
Comment on attachment 8726346 [details] [diff] [review]
1248303.patch

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

Fwiw, the idea behind passing these options to the resume request is that the original specification of the protocol had the notion of a resume limit, which specified until what point we should resume (i.e. until the end of a function, until the next line, etc.), and resuming until we hit an exception seemed like a natural extension of that.

If this doesn't make sense to you, I'm open for discussing it. In the meantime, the patch looks good to me. Thanks for looking into this so quickly!
Attachment #8726346 - Flags: review?(ejpbruel) → review+
I think the problem is that this is a config variable that persists and affects the rest of the entire session. So yes, I think it's awkward that it's passed in for resume. The API in dbg-client sort of exposes this awkward-ness. I'd rather just see it work like a normal `reconfigure` request.
Previous try was busted for unrelated reasons https://treeherder.mozilla.org/#/jobs?repo=try&revision=6680598dad1c

(thanks for the quick review!)
This is now affecting the current beta version.

What would be required to get the patch out?
Flags: needinfo?(jlong)
Try was still busted. Trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b659545431dd
Flags: needinfo?(jlong)
https://hg.mozilla.org/mozilla-central/rev/14712d1923d4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8726346 [details] [diff] [review]
1248303.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1132501
[User impact if declined]: A core feature of the debugger doesn't work, so if using the devtools it won't properly break on exception
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: not much risk, only in the devtools
[String/UUID change made/needed]:
Attachment #8726346 - Flags: approval-mozilla-aurora?
Bob, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(bob+bmo)
James, could you please describe the test coverage in your uplift request? Have you manually verified the fix? Are there automated tests to catch such regressions in future? Without the verification bit, I cannot approve the uplift. Thanks!
Flags: needinfo?(jlong)
(In reply to Ritu Kothari (:ritu) from comment #17)
> Bob, could you please verify this issue is fixed as expected on a latest
> Nightly build? Thanks!

Just tested, works as expected on current Nightly 48.0a1 (2016-03-21) 20160321030217.

Thanks for the fix, James.
Status: RESOLVED → VERIFIED
Flags: needinfo?(bob+bmo)
Actually, not sure I should have set the status like that... sorry if that was wrong. I'm not able to undo it.
(In reply to Ritu Kothari (:ritu) from comment #18)
> James, could you please describe the test coverage in your uplift request?
> Have you manually verified the fix? Are there automated tests to catch such
> regressions in future? Without the verification bit, I cannot approve the
> uplift. Thanks!

There are no new tests because it would involve opening the toolbox twice and it would be a very slow test for a small fix. However I can add one if needed. We are also refactoring this part of the code and adding tests along the way. If it's required for uplift I will do it.
Flags: needinfo?(jlong)
(In reply to James Long (:jlongster) from comment #21)
> (In reply to Ritu Kothari (:ritu) from comment #18)
> > James, could you please describe the test coverage in your uplift request?
> > Have you manually verified the fix? Are there automated tests to catch such
> > regressions in future? Without the verification bit, I cannot approve the
> > uplift. Thanks!
> 
> There are no new tests because it would involve opening the toolbox twice
> and it would be a very slow test for a small fix. However I can add one if
> needed. We are also refactoring this part of the code and adding tests along
> the way. If it's required for uplift I will do it.

Thanks! Given that this was verified by the bug opener, I will approve the uplift. In future, please mention whether you have tested the patch locally or not in the absence of automated test coverage. Also, adding automated tests is not a must for uplift but highly recommended as it has a huge ROI and impact towards improving Firefox quality.
(In reply to Bob from comment #20)
> Actually, not sure I should have set the status like that... sorry if that
> was wrong. I'm not able to undo it.

Thanks for a prompt verification! You did the right thing by changing bug resolution and nightly 48 status to verified. :)
Comment on attachment 8726346 [details] [diff] [review]
1248303.patch

Regression that was verified as fixed, Aurora47+
Attachment #8726346 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi James, given that Beta 46 is also affected, should we consider uplifting to beta as well? If yes, please nominate.
Flags: needinfo?(jlong)
Comment on attachment 8726346 [details] [diff] [review]
1248303.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1132501
[User impact if declined]: A core feature of the debugger doesn't work, so if using the devtools it won't properly break on exception
[Describe test coverage new/current, TreeHerder]: Verified fix on nightly, no new reported errors.
[Risks and why]: not much risk, only in the devtools
[String/UUID change made/needed]:
Flags: needinfo?(jlong)
Attachment #8726346 - Flags: approval-mozilla-beta?
Comment on attachment 8726346 [details] [diff] [review]
1248303.patch

Fix for dev tools debugger. Please uplift for the beta 5 build.
Attachment #8726346 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Works on Aurora 47.0a2 (2016-03-24) 20160324004030
has problems on uplift to beta:

grafting 335678:dbb7628d3963 "Bug 1248303 - respect break on exceptions flag in subsequent debugger instances. r=ejpbruel, a=ritu"
merging devtools/client/framework/attach-thread.js
merging devtools/shared/client/main.js
warning: conflicts while merging devtools/shared/client/main.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(jlong)
James, can you have a look and if you can fix the issue, maybe land it on beta ?
Hm. I did find an edge case where it still doesn't work on both Nightly and Aurora.

1. Enable pause on exceptions
2. Close dev tools
3. Load test page (http://codepen.io/anon/pen/jWJExa)
4. Open dev tools (F12) *in the middle of the page load*
5. Click the exception button. Exception is reported to console but not paused.

I've repeated this a couple times and it seems reproducible. It is *very* timing dependent. Opening devtools when the output iframe has a grey background, before the "Loading..." text appears, seems to have the highest chance of hitting it. Easier to hit on a new profile. In this case, toggling the pause option does nothing but reopening devtools fixes it.

It's so rare (without intentionally trying to hit it) that I'm not even sure if it's worth trying to fix this particular one. I haven't been able to reproduce on current stable, but finding a regression range sounds like a nightmare.
Let me look more into this before landing on beta. I will follow-up soon.
Flags: needinfo?(jlong)
(In reply to Bob from comment #32)
> Hm. I did find an edge case where it still doesn't work on both Nightly and
> Aurora.
> 
> 1. Enable pause on exceptions
> 2. Close dev tools
> 3. Load test page (http://codepen.io/anon/pen/jWJExa)
> 4. Open dev tools (F12) *in the middle of the page load*
> 5. Click the exception button. Exception is reported to console but not
> paused.
> 
> I've repeated this a couple times and it seems reproducible. It is *very*
> timing dependent. Opening devtools when the output iframe has a grey
> background, before the "Loading..." text appears, seems to have the highest
> chance of hitting it. Easier to hit on a new profile. In this case, toggling
> the pause option does nothing but reopening devtools fixes it.
> 
> It's so rare (without intentionally trying to hit it) that I'm not even sure
> if it's worth trying to fix this particular one. I haven't been able to
> reproduce on current stable, but finding a regression range sounds like a
> nightmare.

I haven't been able to reproduce this once. I can't imaging anything in this code being timing-dependent; when the toolbox opens it configures the "pause on exceptions" feature. If you click the button after opening the tools, it will respect the current config. The only timing-sensitive thing here is if you click the button before the toolbox fully finishes initializing, which makes sense, but I doubt that's what you saw.

There may be an obscure race condition bug somewhere, but that's a lot better than this not working, so I'll rebase on beta and still uplift it there.
This has already been approved, but I don't know how to copy the approval flag. This rebased patch should apply.
Attachment #8735491 - Flags: approval-mozilla-beta?
Comment on attachment 8735491 [details] [diff] [review]
1248303-beta.patch

Approved the 2nd patch retroactively  :)
Attachment #8735491 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
Not able to find the "Test exceptions" button

STR: clear.


Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Steps working due to not able to find the button, test is incomplete.

Actual Results: 
As expected
(In reply to Mayur Patil from comment #38)
> Comments:
> Not able to find the "Test exceptions" button

Open the codepen link from the initial comment. The test button will render below the "HTML" box. It might not be visible if your screen/window is very small.
Tested working on Beta 46.0b9, 20160407053945.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.