1108042 - Unblackboxing automatically black boxed sources does not persist after refresh

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

1) Load yahoo.com
2) Open debugger. Observe that sfext-min.js is blackboxed (auto-blackboxing of minified JS needs to be enabled)
3) Un-blackbox it
4) Reload yahoo.com

bug: it's back to blackboxed state

Comment 1

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

In the source actor factory function `ThreadSources.prototype.source`:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#5289

    if (this._autoBlackBox && this._isMinifiedURL(actor.url)) {
      this.blackBox(actor.url);
    }

We need some kind of check for either (a) we've already done the isMinified test for this actor/source/url/whatever and don't need to go through this stuff again, or (b) the user explicitly un-blackboxed this source, so we should stop considering it here.

I'm leaning towards (a).

Comment 2

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

(The "minified script" was important because it is getting automatically black boxed).

Comment 3

[Zimon Dai [:zimonD]]

Cannot reproduce in latest Nightly 38.0a1(20150203, https://hg.mozilla.org/mozilla-central/rev/ae5d04409cd9)

Comment 4

[farhaan]

Hi Nick, 
I am new to open source. I am interested in fixing bugs and want to contribute to mozilla. Can you assign it to me and guide me to fix it.

Thanks in advance,
Farhaan

Comment 5

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

(In reply to farhaan from comment #4)
> Hi Nick, 
> I am new to open source. I am interested in fixing bugs and want to
> contribute to mozilla. Can you assign it to me and guide me to fix it.
> 
> Thanks in advance,
> Farhaan

Hi farhaan, I will assign you the bug once you attach a WIP patch for me to take a look at.

See https://wiki.mozilla.org/DevTools/Hacking for general information on how to hack on the devtools.

See comment 1 with info on fixing this specific bug.

Feel free to comment here if you have any specific questions, and I'll do my best to help explain! You can check the "Need more information from mentor" box at the bottom to be sure that I notice :)

Comment 6

[James Long (:jlongster)]

rtorruellas, this would probably be the best place to start actually. Let me know if you want to take this bug!

Comment 7

[Vikram Jadhav]

Yes, I would definitely like to work in this bug as a starter! :)
I have completed the environment building process.
Whats the next step?
Thanks.

Comment 8

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

(In reply to Vikram Jadhav from comment #7)
> Yes, I would definitely like to work in this bug as a starter! :)
> I have completed the environment building process.
> Whats the next step?
> Thanks.

See comment 5. I'll assign the bug to the first person to put up a work-in-progress patch.

Comment 9

[Vikram Jadhav]

Attachment: Bug_1108042_Unblackboxing_automatically solved

Comment 10

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

Comment on attachment 8573288 [details] [diff] [review]
Bug_1108042_Unblackboxing_automatically solved

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

This needs a devtools-mochitest that does the following:

* Loads a test page with a minified source

* Verifies that source is automatically black boxed

* Un-blackboxes the source

* Refreshes

* Verifies that the source is still un-blackboxed

You can learn more about devtools tests here: https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests

A good test to base your new test off of is browser/devtools/debugger/test/browser_dbg_blackboxing-01.js

Make sure to add your new test to browser/devtools/debugger/test/browser.ini or else it won't integrate into the infrastructure.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No updates in many months, unassigning.

Comment 12

[MAERTEN Ahri]

Attachment: No autoblackboxing of minified sources when they've been already checked

Comment 13

[MAERTEN Ahri]

Attachment: No autoblackboxing of minified sources when they've been already checked

Comment 14

[MAERTEN Ahri]

Attachment: bug-1108042.patch

Modified TabSources.js + mochitest

Comment 15

[James Long (:jlongster)]

Comment on attachment 8692009 [details] [diff] [review]
bug-1108042.patch

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

::: devtools/server/actors/utils/TabSources.js
@@ +32,5 @@
>      return !isHiddenSource(source) && allowSourceFn(source);
>    }
>  
>    this.blackBoxedSources = new Set();
> +  this.checkedMinifiedURLS = new Set();

Thanks for the patch! Can you find a better name for this? Something that reflects that we want to just never auto-blackbox it.

Comment 16

[MAERTEN Ahri]

Attachment: bug-1108042.patch

New name for this Set (I think I'm very bad with name. I hope it's better and not too verbose ^^)

Comment 17

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8692120 [details] [diff] [review]
bug-1108042.patch

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

This patch looks great. I just have two minor comments. Other than that, it looks good to go. Well done!

You can either put up a new patch with my two comments addressed, or I can address the comments for you. Your choice.

Now that you have an r+, the next step is to push this patch to our try server, to make sure we didn't break any tests. I assume you don't have access to our try server yet, so I can do that for you. Once you have a green test run on the try server, you can request your patch to be checked in, by setting the checkin flag to ?

::: devtools/client/debugger/test/mochitest/browser.ini
@@ +106,5 @@
>    doc_split-console-paused-reload.html
>    doc_step-many-statements.html
>    doc_step-out.html
>    doc_terminate-on-tab-close.html
> +  doc_user_unblackbox.html

Could we give this a similar name to code_blackboxing_unblackbox.min.js? That should make it easier to tell that these files are related to each other. doc_blackboxing_unblackbox.html would work, for instance.

::: devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-07.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Test that if we black box a source and then refresh, it is still black boxed.

It's the other way around. We unblackbox a source. And not just any source, an automatically blackboxed source. Please update the comment to reflect this.

Comment 18

[Eddy Bruel [:ejpbruel]]

Try run for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a425dae829e

Comment 19

[MAERTEN Ahri]

Attachment: bug-1108042.patch

Better name for the html support file
Fixed comment to reflect what the mochitest is doing

Comment 20

[Pulsebot]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d69591dd06dd