Last Comment Bug 762761 - An option to deobfuscate javascripts in the debugger
: An option to deobfuscate javascripts in the debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: Other All
: P2 normal with 5 votes (vote)
: Firefox 27
Assigned To: Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
:
: James Long (:jlongster)
Mentors:
: 767177 873347 (view as bug list)
Depends on: 918821 568142 669999 759837 908913 914930 918797 918802
Blocks: 815280 913665 917072
  Show dependency treegraph
 
Reported: 2012-06-07 19:37 PDT by Salar Khalilzadeh
Modified: 2014-02-09 04:53 PST (History)
25 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
27+


Attachments
part 1: server / client changes (WIP) (13.51 KB, patch)
2013-09-04 17:03 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Splinter Review
part 1: server / client changes (WIP) (2.17 MB, patch)
2013-09-06 14:05 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
no flags Details | Diff | Splinter Review
part 1: server / client changes (26.55 KB, patch)
2013-09-09 14:09 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
past: review+
Details | Diff | Splinter Review
part 2: front end / UI changes (28.28 KB, patch)
2013-09-16 17:20 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
vporof: review+
Details | Diff | Splinter Review
part 3: fix the button's label (1.18 KB, patch)
2013-09-18 18:10 PDT, Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
nfitzgerald: review+
Details | Diff | Splinter Review

Description Salar Khalilzadeh 2012-06-07 19:37:26 PDT
Since most javascript codes in websites are obfuscated, it is essential to have a deobfuscator inside the debugger. Afterward debugging the deobfuscated script should be possible.
Similar to Pretty Print option in chrome.

Here is some sample:
http://jsbeautifier.org/
https://addons.mozilla.org/en-us/firefox/addon/javascript-deobfuscator/
Comment 1 Mardeg 2012-06-07 19:44:54 PDT
Related to bug 670002
Comment 2 Mardeg 2012-06-07 19:48:14 PDT
This is likely a dupe to one of the bugs linked to from
https://wiki.mozilla.org/DevTools/Features/SourceMap
Comment 3 Salar Khalilzadeh 2012-06-07 20:35:40 PDT
Yes it is actually kinda same except for one thing,
I didn't see in any place mentioned that the code should be displayed deobfuscated to user and the user should be able to debug line by line in the deobfuscated code.
If it is already mentioned or planned, go ahead and mark this as duplicate.
Comment 4 Victor Porof [:vporof][:vp] 2012-06-08 02:02:34 PDT
We can use SpiderMonkey's parser API to prettify js. When (if) that's removed and esprima is nicely integrated, we could also switch to that.

Panos, do you think this is a good idea? I could play with it a bit.
Comment 5 Panos Astithas [:past] 2012-06-08 02:18:04 PDT
(In reply to Victor Porof from comment #4)
> We can use SpiderMonkey's parser API to prettify js. When (if) that's
> removed and esprima is nicely integrated, we could also switch to that.
> 
> Panos, do you think this is a good idea? I could play with it a bit.

Yes, I think it's a good idea, but I don't know how we should go about prioritizing this work against the source map support. We need feedback from Rob.
Comment 6 Kevin Dangoor 2012-06-08 08:15:24 PDT
From talking with Rob and Dave about this somewhat recently, it sounded like there may be some parts of the implementation that could be common between this and sourcemaps.

I did create a rough feature page for this a few days ago: https://wiki.mozilla.org/DevTools/Features/ReadableDebug and it's good to have a bug to track this.

Prettifying JS is more generally useful today than sourcemaps because:

1. sourcemap support is just getting going. I'm not sure if anything other than Closure Compiler supports them yet

2. even with sourcemaps for one's own code, having a prettifier will make exploring code on random, deployed sites more manageable

With sourcemaps, it's something of a "chicken and egg" problem. The more browser support there is for sourcemaps, the more tools will output them. Still, in the meantime, being able to prettify code and set breakpoints against that prettified code will be a big win.
Comment 7 Rob Campbell [:rc] (:robcee) 2012-06-14 09:25:13 PDT
let's take a look at this with our parser when it's available. Does not depend on sourcemaps per se (though source mapped js files will benefit from that obviously).
Comment 8 Kevin Dangoor 2012-06-21 19:42:10 PDT
*** Bug 767177 has been marked as a duplicate of this bug. ***
Comment 9 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-06-22 04:15:18 PDT
If a minified script doesn't have a source map and we prettify it, we need to generate a source map mapping minified code -> prettified code to be able to continue debugging the prettified code.

cc'ing dcamp because we just talked about this feature when he was in MV.
Comment 10 Panos Astithas [:past] 2012-06-22 04:24:37 PDT
(In reply to Nick Fitzgerald :fitzgen from comment #9)
> If a minified script doesn't have a source map and we prettify it, we need
> to generate a source map mapping minified code -> prettified code to be able
> to continue debugging the prettified code.

That would be great. Although prettifying without generating a source map wouldn't be a regression, since a minified script cannot be debugged without column support in SpiderMonkey.
Comment 11 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2012-06-22 14:04:23 PDT
(In reply to Panos Astithas [:past] from comment #10)
> (In reply to Nick Fitzgerald :fitzgen from comment #9)
> > If a minified script doesn't have a source map and we prettify it, we need
> > to generate a source map mapping minified code -> prettified code to be able
> > to continue debugging the prettified code.
> 
> That would be great. Although prettifying without generating a source map
> wouldn't be a regression, since a minified script cannot be debugged without
> column support in SpiderMonkey.

Ah yes, of course.
Comment 12 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 08:10:48 PDT
Also see bug 640631
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2012-07-10 08:18:01 PDT
And https://addons.mozilla.org/en-US/firefox/addon/javascript-deminifier/
Comment 14 Victor Porof [:vporof][:vp] 2012-12-13 23:38:07 PST
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #13)
> And https://addons.mozilla.org/en-US/firefox/addon/javascript-deminifier/

Doesn't work with our debugger :(
Comment 15 Panos Astithas [:past] 2013-05-16 22:40:38 PDT
*** Bug 873347 has been marked as a duplicate of this bug. ***
Comment 16 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-04 17:03:28 PDT
Created attachment 799856 [details] [diff] [review]
part 1: server / client changes (WIP)

Works when you aren't pretty printing a source that is already source mapped. Probably doesn't work with stepping yet.
Comment 17 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-06 14:05:12 PDT
Created attachment 800962 [details] [diff] [review]
part 1: server / client changes (WIP)

works with stepping and all that now, but not with other source maps yet.
Comment 18 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-09 14:09:14 PDT
Created attachment 801780 [details] [diff] [review]
part 1: server / client changes

Works with source mapped sources as well now.

https://tbpl.mozilla.org/?tree=Try&rev=f714822147e7
Comment 19 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-09 15:25:05 PDT
Pull request to the debugger docs repo for these protocol changes: https://github.com/jimblandy/DebuggerDocs/pull/16
Comment 20 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-09 15:28:19 PDT
Pull request for the minor source-map lib changes included in this patch: https://github.com/mozilla/source-map/pull/77
Comment 21 Panos Astithas [:past] 2013-09-10 13:01:44 PDT
Comment on attachment 801780 [details] [diff] [review]
part 1: server / client changes

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

The code changes are lovely! The only question I'd like to raise is whether "prettyPrint" is the right request/method name here or something like "deobfuscate" or "unminify" would be better.
Comment 22 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-10 13:16:31 PDT
I think that prettyPrint is a pretty well understood verb for this kind of thing, and is the only non-negation (ha). Also, it isn't necessarily what we have to call it in the UI exposed in the front end.
Comment 23 Anthony Ricaud (:rik) 2013-09-10 13:38:12 PDT
I think this is also a good name because web developers may want to pretty-print a JS file that is just poorly indented (so it's not about deobfuscating or unminifying).
Comment 24 Girish Sharma [:Optimizer] 2013-09-10 13:42:36 PDT
What about calling it "Prettify" on the front end (in the UI). I know chrome calls it "Pretty print", but here, we are not just printing the file, its live code, which you can play with (and possibly edit in future too).

Pretty print also looks fine. I am just laying out the thought for other positive sounding choices :)
Comment 25 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-11 10:16:32 PDT
Landed part 1: https://hg.mozilla.org/integration/fx-team/rev/9929cf38b486
Comment 26 Ryan VanderMeulen [:RyanVM] 2013-09-11 19:05:14 PDT
https://hg.mozilla.org/mozilla-central/rev/9929cf38b486
Comment 27 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-16 17:20:08 PDT
Created attachment 805709 [details] [diff] [review]
part 2: front end / UI changes

Talked with Anton/Benvie/Rob about UI. Conclusions:

* Add context menu, because a lot of people expect that.

* Add a new mini toolbar specifically for sources / selected source. Black boxing, prettifying, enable/disable breakpoints, add new source (scratchpad-in-the-debugger). We need a UI that scales to present all these things to the user. Having a mini toolbar specifically for the sources is one answer, and since uplift just happened, we have a whole cycle to iterate on it.

https://tbpl.mozilla.org/?tree=Try&rev=b9051db5d7be
Comment 28 Victor Porof [:vporof][:vp] 2013-09-18 00:43:20 PDT
Comment on attachment 805709 [details] [diff] [review]
part 2: front end / UI changes

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

This is really sweet, thank you Nick!

I would love it if you added a test to make sure that adding breakpoints and stepping work properly in the frontend after prettification. There's also the interesting case of searching for functions in the filtered source resulting in incorrect offsets, which is easy to fix, but should also be tested IMHO.

r+ with the comments addressed.

::: browser/devtools/debugger/debugger-controller.js
@@ +1145,5 @@
>      });
>    },
>  
>    /**
> +   * Pretty print a source's text.

Might want to add a comment that any subsequent requests for the source's text via getText() would offer the prettified source instead of the original source. Add a test for it too.

@@ +1160,5 @@
> +    if (textPromise && textPromise.pretty) {
> +      return textPromise;
> +    }
> +
> +    const d = promise.defer();

I know this is d for "deferred", but I don't see any good reason for not using a descriptive identifier.

@@ +1172,5 @@
> +        }
> +
> +        if (this.activeThread.paused) {
> +          // Update the stack frame list.
> +          this.activeThread._clearFrames();

Can you take this opportunity to make clearFrames a public method on the thread client? It's being used in lots of places.

@@ +1174,5 @@
> +        if (this.activeThread.paused) {
> +          // Update the stack frame list.
> +          this.activeThread._clearFrames();
> +          this.activeThread.fillFrames(CALL_STACK_PAGE_SIZE);
> +        }

You'll also have to remove the cached source text from the Parser, to avoid getting wrong locations when searching for functions.

There's a clearCache method on the Parser object in Parser.jsm, which you shouldn't use because it blows away all ASTs. So add a new method there, clearSource or which only removes a specific source.

::: browser/devtools/debugger/debugger-panes.js
@@ +380,5 @@
> +   */
> +  prettyPrint: function() {
> +    const resetEditor = () => {
> +      // Only set the text when the source is still selected.
> +      if (this.selectedItem.attachment.source.url === source.url) {

Use this.selectedValue instead of that this.that.that.that..

@@ +389,5 @@
> +    let { source } = this.selectedItem.attachment;
> +    DebuggerController.SourceScripts.prettyPrint(source)
> +      .then(resetEditor,
> +            // Reset the editor even when we fail, so that we can give the user
> +            // a clue as to why the source isn't pretty printed and what

Nit: this is some funky ass indenting. Maybe you could move the comment above the call?

::: browser/devtools/debugger/debugger-view.js
@@ +272,5 @@
>     * @param object aSource
>     *        The source object coming from the active thread.
> +   * @param object aFlags
> +   *        Additional options for setting the source. Supported options:
> +   *          - reset: boolean allowing whether we can get the selected url's

How about using "force" as a flag, since it's more consistent with other options for other methods.

@@ +336,5 @@
>     *          - columnOffset: column offset for the caret or debug location
>     *          - noCaret: don't set the caret location at the specified line
>     *          - noDebug: don't set the debug location at the specified line
> +   *          - reset: boolean allowing whether we can get the selected url's
> +   *                   text again.

Ditto.

::: browser/devtools/debugger/debugger.xul
@@ +327,5 @@
> +            <toolbarbutton id="pretty-print"
> +                           tooltiptext="&debuggerUI.sources.prettyPrint;"
> +                           class="devtools-toolbarbutton devtools-monospace"
> +                           command="prettyPrintCommand">
> +              {}

I think the text for such buttons should be in a "value" attribute, to avoid XUL wrapping warnings. Also, I don't know if we should we localize this? Probably not? Ask flod to be sure.

::: browser/devtools/debugger/test/browser_dbg_pretty-print-01.js
@@ +23,5 @@
> +    gEditor = gDebugger.DebuggerView.editor;
> +
> +    waitForSourceShown(gPanel, "code_ugly.js")
> +      .then(clickPrettyPrintButton)
> +      .then(() => waitForSourceShown(gPanel, "code_ugly.js"))

waitForSourceShown starts listening for the respective event. In the case that the source is shown *before* this function is called, the test will get stuck and timeout here. You can either:

then(() => {
  let finished = waitForSourceShown(gPanel, "code_ugly.js");
  clickPrettyPrintButton();
  return finished;
})

or:

.then(clickPrettyPrintButton)
.then(() => ensureSourceIs(gPanel, "code_ugly.js", true))

...the latter doing exactly what you'd expect.

@@ +40,5 @@
> +}
> +
> +function testSourceIsPretty() {
> +  ok(gEditor.getText().contains("\n    "),
> +     "The source should be pretty printed.")

Nit: this seems a bit fragile. How about making sure that it fails found before prettification?

::: browser/devtools/debugger/test/browser_dbg_pretty-print-02.js
@@ +21,5 @@
> +    gContextMenu = gDebugger.document.getElementById("sourceEditorContextMenu");
> +
> +    waitForSourceShown(gPanel, "code_ugly.js")
> +      .then(selectContextMenuItem)
> +      .then(waitForSourceShown.bind(null, gPanel, "code_ugly.js"))

Ditto here like in the last test.

@@ +40,5 @@
> +}
> +
> +function testSourceIsPretty() {
> +  ok(gEditor.getText().contains("\n    "),
> +     "The source should be pretty printed.")

Ditto.

::: browser/devtools/debugger/test/browser_dbg_pretty-print-03.js
@@ +7,5 @@
> +
> +const TAB_URL = EXAMPLE_URL + "doc_pretty-print.html";
> +
> +let gTab, gDebuggee, gPanel, gDebugger;
> +let gFrames, gVariables, gPrefs, gOptions;

You're never using the gVariables, gPrefs or gOptions globals.

@@ +24,5 @@
> +    waitForSourceShown(gPanel, "code_ugly.js")
> +      .then(runCodeAndPause)
> +      .then(clickPrettyPrintButton)
> +      .then(waitForSourceShown.bind(null, gPanel, "code_ugly.js"))
> +      .then(waitForCaretUpdated.bind(null, gPanel, 7))

Ditto here for "waiting potentially too late". You can use ensureSourceIs is and ensureCaretAt, or the more verbose

then(() => {
  let finished = waitForSourceAndCaret(gPanel, "code_ugly.js", 7);
  clickPrettyPrintButton();
  return finished;
})

@@ +36,5 @@
> +function runCodeAndPause() {
> +  const d = promise.defer();
> +  gDebugger.DebuggerController.activeThread.addOneTimeListener(
> +    "paused",
> +    () => d.resolve(true));

You can safely use "once" here for a more compact line:

once(gDebugger.gThreadClient, "paused", d.resolve)

@@ +37,5 @@
> +  const d = promise.defer();
> +  gDebugger.DebuggerController.activeThread.addOneTimeListener(
> +    "paused",
> +    () => d.resolve(true));
> +  executeSoon(gDebuggee.foo);

Please add a comment here explaining the executeSoon. I think it's in everyone's benefit if such tick/time-dependent things are explained. This is the comment used in other tests for this particular case:

// Spin the event loop before causing the debuggee to pause, to allow
// this function to return first.

::: browser/devtools/debugger/test/doc_pretty-print.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<script src="code_ugly.js"></script>

Nit: add a meta charset tag here to avoid warnings.
Comment 29 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-18 17:56:56 PDT
https://hg.mozilla.org/integration/fx-team/rev/cc0aa7f1f41f
Comment 30 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2013-09-18 18:10:25 PDT
Created attachment 807008 [details] [diff] [review]
part 3: fix the button's label

Switching from putting "{}" in a text node to the value attribute broke the button's text. After looking at MDN, the label attribute was what you were thinking of, but it adds way too much padding, so I am just reverting to text node again.

https://hg.mozilla.org/integration/fx-team/rev/0a8d751016f9
Comment 31 Victor Porof [:vporof][:vp] 2013-09-19 07:24:56 PDT
(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> Created attachment 807008 [details] [diff] [review]
> part 3: fix the button's label
> 

Easily fixable via CSS or the "plain" default style IIRC.
Filed bug 918329
Comment 33 bhavana bajaj [:bajaj] 2013-10-25 13:10:24 PDT
Add it as a dev note on release notes.

Note You need to log in before you can comment on or make changes to this bug.