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)
DevTools
Debugger
Tracking
(firefox44 unaffected, firefox46+ fixed, firefox47+ verified, firefox48 verified)
VERIFIED
FIXED
Firefox 48
People
(Reporter: bob+bmo, Assigned: jlong)
References
Details
(Keywords: regression)
Attachments
(2 files)
4.08 KB,
patch
|
ejpbruel
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
3.99 KB,
patch
|
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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?
status-firefox44:
--- → unaffected
status-firefox46:
--- → affected
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.
Aurora regression: https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=3bfa5bc61b626761d487b45c170b115259f69d6b&tochange=b9a803752a2cb143582e6665ed3fb679eebf60b3 Nightly regression: https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=3bfa5bc61b626761d487b45c170b115259f69d6b&tochange=b9a803752a2cb143582e6665ed3fb679eebf60b3 fx-team regression: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=89c1cc2faea8903570688dd38d9f71ec0c5f0b87&tochange=256e3e81fed77b17ea9158786a349698bb59a6ab Changeset: https://hg.mozilla.org/integration/fx-team/rev/256e3e81fed7 Looks like it's due to the fix for bug 1132501
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.
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jlong
Flags: needinfo?(jlong)
Comment 4•8 years ago
|
||
Recent regression, tracking. James once you have a viable fix please request uplift to aurora 46. Thanks!
Comment 5•8 years ago
|
||
This is a regression that breaks existing functionality. It should be fixed asap. Marking as P1.
Priority: -- → P1
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40158908ec27
Assignee | ||
Comment 8•8 years ago
|
||
Eddy, we should try to get this in before the uplift next week
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
Previous try was busted for unrelated reasons https://treeherder.mozilla.org/#/jobs?repo=try&revision=6680598dad1c (thanks for the quick review!)
Reporter | ||
Comment 12•8 years ago
|
||
This is now affecting the current beta version. What would be required to get the patch out?
Flags: needinfo?(jlong)
Assignee | ||
Comment 13•8 years ago
|
||
Try was still busted. Trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b659545431dd
Flags: needinfo?(jlong)
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/14712d1923d4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 16•8 years ago
|
||
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)
Reporter | ||
Comment 19•8 years ago
|
||
(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.
Reporter | ||
Comment 20•8 years ago
|
||
Actually, not sure I should have set the status like that... sorry if that was wrong. I'm not able to undo it.
Assignee | ||
Comment 21•8 years ago
|
||
(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)
Assignee | ||
Comment 26•8 years ago
|
||
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 27•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbb7628d3963
Comment 28•8 years ago
|
||
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+
Reporter | ||
Comment 29•8 years ago
|
||
Works on Aurora 47.0a2 (2016-03-24) 20160324004030
Comment 30•8 years ago
|
||
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)
Comment 31•8 years ago
|
||
James, can you have a look and if you can fix the issue, maybe land it on beta ?
Reporter | ||
Comment 32•8 years ago
|
||
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.
Assignee | ||
Comment 33•8 years ago
|
||
Let me look more into this before landing on beta. I will follow-up soon.
Flags: needinfo?(jlong)
Assignee | ||
Comment 34•8 years ago
|
||
(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.
Assignee | ||
Comment 35•8 years ago
|
||
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 36•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/6d788aecf2c7
Comment 37•8 years ago
|
||
Comment on attachment 8735491 [details] [diff] [review] 1248303-beta.patch Approved the 2nd patch retroactively :)
Attachment #8735491 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 38•8 years ago
|
||
[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
Reporter | ||
Comment 39•8 years ago
|
||
(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.
Reporter | ||
Comment 40•8 years ago
|
||
Tested working on Beta 46.0b9, 20160407053945.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•