1258821 - Cannot open the expression panel of conditional breakpoint

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[magicp]

Attachment: conditional-breakpoint-expression-panel.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160322030417

Steps to reproduce:

1. Start Nightly
2. Go to any sites (e.g. about:home)
3. Open Debugger
4. Add two or more conditional breakpoint
5. Open the expression panel selecting conditional breakpoint detail
6. Select next conditional breakpoint detail remain opening the expression panel


Actual results:

Cannot open the expression panel of conditional breakpoint. (cannot keep open, because immediately close automatically)

Reproduce on Nightly, but other versions will have same issue. Because "Add conditional breakpoint" does not work correctly in other versions now.


Expected results:

The expression panel should open when selecting conditional breakpoint detail.

Comment 1

[Eddy Bruel [:ejpbruel]]

This looks like a regression. Jlast, could this be fallout from bug 900763?

In any case, this should be fixed asap. Marking as p1.

Comment 2

[Jason Laster [:jlast]]

Looking into it now ejbruel. It seems likely that bug 900763 caused it.

Comment 3

[Jason Laster [:jlast]]

Actually ejbruel, magicp, it looks like the regression was masked by bug 1256704, which was breaking conditional breakpoints entirely. I reverted the patch from 900763 and the regression was still there. 

I'm going to continue to look into the bug and see if I can get to the bottom of it.

Comment 4

[Jason Laster [:jlast]]

I believe the issue is the popup's opening/closing animation. Basically, we open the second popup immediately, and when the first popup is done animating away we close both of them. 

One way to see this happening is to set a breakpoint here: 
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/content/views/sources-view.js#L687
and evaluating _cbPanel.getAttribute('animation')

This patch fixes it https://gist.github.com/jasonLaster/2768b58da831460e7f57, but uses a hard coded value. I'm going to see if I can observe the animation attribute changing, which avoid the hard coded value.

Comment 5

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1eadf5c9886

Comment 6

[Jason Laster [:jlast]]

Attachment: cpopup.1.patch

Problem: the conditional breakpoint panel is shared between breakpoints. When you try to see a breakpoint expression and the panel is already open it breaks. (see attachment 1 [details] [diff] [review]). It breaks because the panel is animating out while attempting to show the new expression at the same time.

The fix is to wait on opening the panel until the panel has been hidden. 

Note, I'm using the `_cbPanel.hidden` property to check if the panel is open or closed. Previously, the hidden property did not have a default. Also, it was set to hidden immediately. This patch fixes those things so the property is reliable.

Comment 7

[James Long (:jlongster)]

Comment on attachment 8735440 [details] [diff] [review]
cpopup.1.patch

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

Looks good! I only looked at the general idea and I assume that the flow works as it should.

::: devtools/client/debugger/content/views/sources-view.js
@@ +696,5 @@
> +      cbPanel.removeEventListener('popuphidden', openPopup, false);
> +    }
> +
> +    // Wait until the other cb panel is closed
> +    if (!this._cbPanel.hidden) {

You assigned `this._cbPanel` to a variable above so you could use that here also

Comment 8

[James Long (:jlongster)]

You should assign yourself to bugs that you work on :)

Comment 9

[Jason Laster [:jlast]]

> You assigned `this._cbPanel` to a variable above so you could use that here also

Yep, that's correct `this._cbPanel.hidden` was not reliable the way it was setup before.

Comment 10

[Carsten Book [:Tomcat]]

has problems to apply:

patching file devtools/client/debugger/content/views/sources-view.js
Hunk #1 FAILED at 59
Hunk #3 FAILED at 144
2 out of 5 hunks FAILED -- saving rejects to file devtools/client/debugger/content/views/sources-view.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh cpopup.1.patch

Comment 11

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd59424d35f

Comment 12

[Jason Laster [:jlast]]

Attachment: cpopup.2.patch

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/646cc46e3cfc

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/646cc46e3cfc