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)

Product:
DevTools
Component:
Debugger
Type:
defect

Tracking

(firefox31 wontfix, firefox32 fixed, firefox33 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 33
Tracking Flags:
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: vporof, Assigned: past)

References

Details

Attachments

(1 file, 2 obsolete files)

Avoid a race in CodeMirror initialization when setting breakpoints and don't let ESC stop navigation if the debugger is paused v2
7.47 KB, 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?
Priority: -- → P3
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.
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: nobody → past
Status: NEW → ASSIGNED
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)
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.
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.
This is really weird. How is the escape key affecting breakpoints?
Is this testable?
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.
My understanding is we do have a support contract for Code Mirror...
OK, I'll ask Rob about it on Monday then.
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)
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
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.
Nice!! How did you manage to track this down?
Attachment #8442829 - Attachment is obsolete: true
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+
(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?
> 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.
Yes, I'm referring to the ESC case specifically.
(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.
Attachment #8450248 - Attachment is obsolete: true
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+
Thank you!
Also, after this lands, can we backport it to at least aurora, maybe even beta? It's annoying as hell.
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]
https://hg.mozilla.org/mozilla-central/rev/ed97e3930c36
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
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 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+
Even if we have a test, asking for a QA check to make sure it is fixed for the user.
Keywords: qawanted, verifyme
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-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: