Closed
Bug 957174
Opened 11 years ago
Closed 10 years ago
Pressing escape while the debugger is paused causes it to lose debugging context
Categories
(DevTools :: Debugger, defect, P3)
DevTools
Debugger
Tracking
(firefox31 wontfix, firefox32 fixed, firefox33 fixed)
RESOLVED
FIXED
Firefox 33
People
(Reporter: vporof, Assigned: past)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
past
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
STR:
1. Open http://todomvc.com/architecture-examples/backbone/
2. Add a breakpoint on line 31 in app-view.js
3. Reload the page
4. Wait for the breakpoint to hit
5. Press escape
Notice how the stackframes breadcrumbs and the editor debug line disappear. The "resume" button in the toolbar now shows a "pause" icon.
This is terribly confusing. Why does this even happen?
Updated•11 years ago
|
Priority: -- → P3
Comment 2•11 years ago
|
||
Not sure if it will be useful, but one thing I noticed when trying to track this down is that if you just pause by pressing the button or pressing F8 (instead of hitting a breakpoint or debugger statement), then pressing escape does not trigger this behavior.
Assignee | ||
Comment 4•10 years ago
|
||
This is certainly a race. I don't understand enough of the innards of CodeMirror to completely figure it out, but this patch seems to fix it.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → past
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8442829 [details] [diff] [review]
Avoid a race in CodeMirror initialization when setting breakpoints
Seems to fix all similar cases, so I guess it's ready for review. I haven't added a test, because I'm not sure triggering the race is going to be 100% reliable and we already have a test that fails Very Often (bug 1016310) without this patch.
Attachment #8442829 -
Flags: review?(vporof)
Comment 7•10 years ago
|
||
Perhaps we should have a follow up with Marijn about fixing the race and removing this ugliness... :-/
Is 100ms too little amount of time? I'm afraid 200ms might be too notice-able.
Assignee | ||
Comment 8•10 years ago
|
||
If he doesn't mind looking at our embedding code, by all means! I'm not even sure what is causing it. Perhaps getting rid of the deprecated-sync-thenables could help some, although it won't completely fix the issue (I tried).
100 ms works too in my testing, I just picked the value we use almost consistently in that file. And since we use 200 ms in a few other places we already need about 200 ms for the UI to settle in many cases.
Reporter | ||
Comment 9•10 years ago
|
||
This is really weird. How is the escape key affecting breakpoints?
Reporter | ||
Comment 10•10 years ago
|
||
Is this testable?
Assignee | ||
Comment 11•10 years ago
|
||
The escape key is completely unrelated. The errors appears when the breakpoint is hit and the debugger tries to update the editor, even without hitting escape.
Comment 12•10 years ago
|
||
My understanding is we do have a support contract for Code Mirror...
Assignee | ||
Comment 13•10 years ago
|
||
OK, I'll ask Rob about it on Monday then.
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8442829 [details] [diff] [review]
Avoid a race in CodeMirror initialization when setting breakpoints
Review of attachment 8442829 [details] [diff] [review]:
-----------------------------------------------------------------
I can still reproduce this with a slight variation of the steps in comment 0.
1. Open http://todomvc.com/architecture-examples/backbone/
2. Add a breakpoint on line 31 in app-view.js
3. Reload the page
4. Wait for the breakpoint to hit
5. Click anywhere in the debugger where focus may be stolen from the editor (like `this` in the variables view, a breadcrumb or a different source)
6. Press escape
Attachment #8442829 -
Flags: review?(vporof)
Reporter | ||
Comment 15•10 years ago
|
||
Addendum for the above comment:
* Funny enough, if you click the 'pause' button in the debugger, it breaks where you left off
* Simply opening the split console without pressing ESC doesn't cause the debugger to lose context
Assignee | ||
Comment 16•10 years ago
|
||
So this is actually unrelated to the split console. What is happening is that pressing ESC triggers the browser's BrowserStop() handler here, which cancels the page load:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#72
That eventually ends up here, where navigation is stopped:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1664
That triggers a tabNavigated:stop packet and the debugger just reacts to it as expected.
I think that we need to block ESC from interfering with page loading when the debugger panel is open and execution is paused. Looking for the best way to do that.
Reporter | ||
Comment 17•10 years ago
|
||
Nice!! How did you manage to track this down?
Assignee | ||
Comment 18•10 years ago
|
||
This seems to work as expected.
Attachment #8450248 -
Flags: review?(vporof)
Assignee | ||
Updated•10 years ago
|
Attachment #8442829 -
Attachment is obsolete: true
Reporter | ||
Comment 19•10 years ago
|
||
Comment on attachment 8450248 [details] [diff] [review]
Avoid a race in CodeMirror initialization when setting breakpoints and don't let ESC stop navigation if the debugger is paused
Review of attachment 8450248 [details] [diff] [review]:
-----------------------------------------------------------------
Can a test be added?
::: browser/devtools/framework/toolbox.js
@@ +294,5 @@
> let responsiveModeActive = this._isResponsiveModeActive();
> if (e.keyCode === e.DOM_VK_ESCAPE && !responsiveModeActive) {
> this.toggleSplitConsole();
> + // If the debugger is paused, don't let the ESC key stop any pending
> + // navigation.
Hacky, but ok.
::: toolkit/devtools/event-emitter.js
@@ +156,5 @@
> + let file = caller.filename;
> + if (file.contains(" -> ")) {
> + file = caller.filename.split(/ -> /)[1];
> + }
> + path = file + ":" + caller.lineNumber;
Why is this here?
Attachment #8450248 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Victor Porof [:vporof][:vp] from comment #19)
> ::: toolkit/devtools/event-emitter.js
> @@ +156,5 @@
> > + let file = caller.filename;
> > + if (file.contains(" -> ")) {
> > + file = caller.filename.split(/ -> /)[1];
> > + }
> > + path = file + ":" + caller.lineNumber;
>
> Why is this here?
It's a fix for an unrelated bug that I stumbled upon while debugging (the EventEmitter logger missing the source filenames in some cases). Do you want me to use a separate bug for it?
Assignee | ||
Comment 21•10 years ago
|
||
> Can a test be added?
Reproducing the CM race is hard, as it is very sensitive to system timings. The ESC fix might be testable, I'll give it a try tomorrow.
Reporter | ||
Comment 22•10 years ago
|
||
Yes, I'm referring to the ESC case specifically.
Reporter | ||
Comment 23•10 years ago
|
||
(In reply to Panos Astithas [:past] from comment #20)
> (In reply to Victor Porof [:vporof][:vp] from comment #19)
> > ::: toolkit/devtools/event-emitter.js
> > @@ +156,5 @@
> > > + let file = caller.filename;
> > > + if (file.contains(" -> ")) {
> > > + file = caller.filename.split(/ -> /)[1];
> > > + }
> > > + path = file + ":" + caller.lineNumber;
> >
> > Why is this here?
>
> It's a fix for an unrelated bug that I stumbled upon while debugging (the
> EventEmitter logger missing the source filenames in some cases). Do you want
> me to use a separate bug for it?
There might be some archeology benefit in landing this separately, since it doesn't seem to be in any way related to this bug.
Assignee | ||
Comment 24•10 years ago
|
||
I managed to write a test for the ESC bug and I've split off the event-emitter.js fix to bug 1034525. Try: https://tbpl.mozilla.org/?tree=Try&rev=9a231764fe13
Assignee | ||
Updated•10 years ago
|
Attachment #8450248 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8451040 [details] [diff] [review]
Avoid a race in CodeMirror initialization when setting breakpoints and don't let ESC stop navigation if the debugger is paused v2
Already reviewed.
Attachment #8451040 -
Flags: review+
Reporter | ||
Comment 26•10 years ago
|
||
Thank you!
Reporter | ||
Comment 27•10 years ago
|
||
Also, after this lands, can we backport it to at least aurora, maybe even beta? It's annoying as hell.
Assignee | ||
Comment 28•10 years ago
|
||
Landed:
https://hg.mozilla.org/integration/fx-team/rev/ed97e3930c36
I'll ask permission for uplift after it bakes in m-c for a few days.
Whiteboard: [fixed-in-fx-team]
Comment 29•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Flags: in-testsuite+
Assignee | ||
Comment 31•10 years ago
|
||
Comment on attachment 8451040 [details] [diff] [review]
Avoid a race in CodeMirror initialization when setting breakpoints and don't let ESC stop navigation if the debugger is paused v2
Approval Request Comment
[Feature/regressing bug #]: there are 2 bugs fixed here, one somewhat recent, the other has been around forever, but was made more likely to occur ever since bug 862558 (Firefox 28)
[User impact if declined]: the debugger will look broken in the case where the page is paused before loading is finished and the user hits ESC
[Describe test coverage new/current, TBPL]: browser_console_optimized_out_vars.js catches the first bug and the new test in the patch catches the second. The last few days in m-c seemed fine.
[Risks and why]: just the general risk that comes with any change, but this one is contained to devtools code, so only developers can encounter any potential regression
[String/UUID change made/needed]: none
Attachment #8451040 -
Flags: approval-mozilla-beta?
Attachment #8451040 -
Flags: approval-mozilla-aurora?
Comment 32•10 years ago
|
||
Comment on attachment 8451040 [details] [diff] [review]
Avoid a race in CodeMirror initialization when setting breakpoints and don't let ESC stop navigation if the debugger is paused v2
Small changes, has tests and 31 is an ESR.
Taking it
Attachment #8451040 -
Flags: approval-mozilla-beta?
Attachment #8451040 -
Flags: approval-mozilla-beta+
Attachment #8451040 -
Flags: approval-mozilla-aurora?
Attachment #8451040 -
Flags: approval-mozilla-aurora+
Comment 33•10 years ago
|
||
Even if we have a test, asking for a QA check to make sure it is fixed for the user.
Comment 34•10 years ago
|
||
Comment on attachment 8451040 [details] [diff] [review]
Avoid a race in CodeMirror initialization when setting breakpoints and don't let ESC stop navigation if the debugger is paused v2
Actually, I changed my mind. It is too late in the beta cycle and we have this bug for a while. Let it ride the train with 32.
If we want it in ESR 31, we can always have it in 31.1.0 or 31.2.0
Attachment #8451040 -
Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Updated•10 years ago
|
Comment 35•10 years ago
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•