967156 - After reloading, the blackbox styles aren't applied to sources in the debugger UI

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

STR:

1. Go to http://todomvc.com/architecture-examples/backbone/
2. Add a breakpoint on line 31 in app-view.js
3. Blackbox that source
4. Reload

Now, the source is indeed blackboxed and the brekapoint didn't trigger, however the breakpoints are still displayed in the UI and the source item isn't grayed out anymore.

Comment 1

[Victor Porof [:vporof][:vp]]

Nick, is this a recent regression? I think this might have worked properly at some point, but I may be wrong.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yeah this used to work; browser_dbg_blackboxing-01.js tests for exactly this case, sad to see that it didn't catch the error.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://wiki.mozilla.org/DevTools/Hacking

Ah this is actually a pretty good first bug. We just need to add the "black-boxed" class to the source element if the source is black boxed in addSource[0]. Make sure to get a source client[1] instead of relying directly on the source form, as that is the real source of truth as sources are black boxed on and off. We should update the test[2] to check for the existence of that css class as well.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#134
[1] |this.activeThread.source(aSource)| should give you a |SourceClient|
[2] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_blackboxing-01.js#42

Comment 4

[Henri Kinnunen]

I would like to work on this one. :)

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Go for it :)

Comment 6

[Victor Porof [:vporof][:vp]]

Need any help with this Henri?

Comment 7

[Mike Hoye [:mhoye]]

Hi, Henri - Can you confirm you're still working on this?

Comment 8

[Henri Kinnunen]

Hi, Sorry I've been really busy for the couple last months. :/

I haven't started working on this yet but I would still like to.

Comment 9

[Mike Hoye [:mhoye]]

I'm going to deassign this, so that it shows up as available to other contributors; you're welcome to work on it, and once you've got a first try at a patch ready to go, I'd be happy to reassign it to you.

Comment 10

[Piyush W]

I tried applying the style as follows:

if (aSource.isBlackBoxed) {
    contents.classList.add("black-boxed");
}

This applies the correct style to the filename in the debugger pane, but the checkbox and the line number don't hide[0] as they should 


[0] http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/debugger.inc.css#110

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to pwaradpande from comment #10)
> I tried applying the style as follows:
> 
> if (aSource.isBlackBoxed) {
>     contents.classList.add("black-boxed");
> }
> 
> This applies the correct style to the filename in the debugger pane, but the
> checkbox and the line number don't hide[0] as they should 
> 
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> debugger.inc.css#110

Sorry for the delayed response, you're on the right path :)

You are applying the class to the <label> element, when it needs to go on the label's containing <vbox> which has class "side-menu-widget-item-contents". If you use the browser toolbox[0] to manually add the class to the label's containing vbox, everything works as expected.

Unfortunately, that vbox isn't created until after you call `push`, so you have to do something like this:

  const item = this.push(...);

  if (gThreadClient.source(aSource).isBlackBoxed) {
    // add the class to item.target
  }

We use gThreadClient.source(...) because it produces a source client whose `isBlackBoxed` getter will stay in sync with whatever black boxing changes may happen. I'm not sure that is an actual issue in this case, but it is good to be vigilant and always use it.

You will also need a test. I'd suggest copying one of the existing browser/devtools/debugger/test/browser_dbg_blackboxing-*.js tests and modifying it. It should:

1. Load a document with two scripts:

  // first script
  function a() {
    b();
  }

  // second script
  function b() {
    debugger;
  }

2. Set a breakpoint in the first script on the line with the call to `b`.

3. Black box the first script containing a.

4. Set up a promise that resolves on the next pause and then refresh.

5. When that promise resolves, verify that you are paused in the second script and b's debugger statement (as opposed to the black boxed breakpoint).

6. Verify that the first script's source item target has the black-boxed css rule and that the breakpoint item is hidden.

For more information about writing devtools mochitests, see [1].

[0] https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
[1] https://wiki.mozilla.org/DevTools/mochitests_coding_standards

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Alternatively, instead of writing a new test, you can just modify this one[0] to also check that the css class is applied properly and the breakpoints are hidden.

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_blackboxing-01.js#42

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Are you still working on this?

Comment 14

[Piyush W]

Well, I have the fix ready. I verified it and it works as expected.

I am stuck at the point of writing test case for the fix. I couldn't find any documentation about what exactly is happening with gDebugee, gTab etc. and what components I need to import. I also had a look at other test files, but I was doing a trial and error. 

Any pointer you could provide for the test case?

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to pwaradpande from comment #14)
> Well, I have the fix ready. I verified it and it works as expected.
> 
> I am stuck at the point of writing test case for the fix. I couldn't find
> any documentation about what exactly is happening with gDebugee, gTab etc.
> and what components I need to import. I also had a look at other test files,
> but I was doing a trial and error. 
> 
> Any pointer you could provide for the test case?

See comment 12. All you should have to do is verify that not only is the button checked, but the css class is applied to the element: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_blackboxing-01.js#42

If you need further clarification, feel free to ping me on irc.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

You shouldn't need to make a new test, just modify that one I linked.

Comment 17

[Piyush W]

Attachment: Adding 'black-boxed' class to the blackboxed source after reload

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8451999 [details] [diff] [review]
Adding 'black-boxed' class to the blackboxed source after reload

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

Looks good!

Here is a try push**: https://tbpl.mozilla.org/?tree=Try&rev=bed24e6610c5

If it comes back green, we will add the "checkin-needed" keyword and one of the sheriffs will commit it (the new version of the patch with requested change below) for us!

** Runs all the tests on our various supported platforms.

::: browser/devtools/debugger/test/browser_dbg_blackboxing-01.js
@@ +43,2 @@
>      ok(bbButton.checked, "Should still be black boxed.");
> +    ok(selectedSource.classList.contains("black-boxed"), "'black-boxed' class should still be applied");

Nitpicking: please put the second parameter on a newline so that this line isn't so long.

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push looks good to me, as soon as you upload the revised patch, we can add "checkin-needed"!

Comment 20

[Piyush W]

Attachment: patch2.patch

Revised patch as per Comment 18

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8452450 [details] [diff] [review]
patch2.patch

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

Just FYI, once you get a review+ with a request for small changes, you don't need to ask for review again.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/8252ef806389

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8252ef806389