Closed Bug 1108042 Opened 5 years ago Closed 4 years ago

Unblackboxing automatically black boxed sources does not persist after refresh

Categories

(DevTools :: Debugger, defect)

35 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: hsteen, Unassigned, Mentored)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 4 obsolete files)

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
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).
Mentor: nfitzgerald
Whiteboard: [good first bug][lang=js]
Summary: un-blackboxing minified script not remembered across refreshes → Unblackboxed sources are not remembered across refreshes
(The "minified script" was important because it is getting automatically black boxed).
Summary: Unblackboxed sources are not remembered across refreshes → Unblackboxing automatically black boxed sources does not persist after refresh
Cannot reproduce in latest Nightly 38.0a1(20150203, https://hg.mozilla.org/mozilla-central/rev/ae5d04409cd9)
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
Flags: needinfo?(nfitzgerald)
(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 :)
Flags: needinfo?(nfitzgerald)
rtorruellas, this would probably be the best place to start actually. Let me know if you want to take this bug!
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.
Flags: needinfo?(nfitzgerald)
(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.
Flags: needinfo?(nfitzgerald)
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.
Attachment #8573288 - Flags: review?(nfitzgerald)
Assignee: nobody → vikramcse.10
Status: NEW → ASSIGNED
No updates in many months, unassigning.
Assignee: vikramcse.10 → nobody
Status: ASSIGNED → NEW
Attachment #8691996 - Attachment is obsolete: true
Attached patch bug-1108042.patch (obsolete) — Splinter Review
Modified TabSources.js + mochitest
Attachment #8691997 - Attachment is obsolete: true
Attachment #8692009 - Flags: review?(ejpbruel)
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.
Attached patch bug-1108042.patch (obsolete) — Splinter Review
New name for this Set (I think I'm very bad with name. I hope it's better and not too verbose ^^)
Attachment #8692009 - Attachment is obsolete: true
Attachment #8692009 - Flags: review?(ejpbruel)
Attachment #8692120 - Flags: review?(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.
Attachment #8692120 - Flags: review?(ejpbruel) → review+
Better name for the html support file
Fixed comment to reflect what the mochitest is doing
Attachment #8692120 - Attachment is obsolete: true
Attachment #8692598 - Flags: checkin+
Attachment #8692598 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/d69591dd06dd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.