Closed Bug 762761 Opened 12 years ago Closed 11 years ago

An option to deobfuscate javascripts in the debugger

Categories

(DevTools :: Debugger, defect, P2)

Other
All
defect

Tracking

(relnote-firefox 27+)

RESOLVED FIXED
Firefox 27
Tracking Status
relnote-firefox --- 27+

People

(Reporter: salar2k, Assigned: fitzgen)

References

Details

Attachments

(3 files, 2 obsolete files)

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/
Related to bug 670002
This is likely a dupe to one of the bugs linked to from
https://wiki.mozilla.org/DevTools/Features/SourceMap
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.
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.
(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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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).
Depends on: 759837
Priority: -- → P3
Depends on: 669999
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.
(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.
(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.
Depends on: 568142
(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 :(
Assignee: nobody → nfitzgerald
Priority: P3 → P2
Depends on: 908913
Status: NEW → ASSIGNED
Whiteboard: [debugger-docs-needed]
Works when you aren't pretty printing a source that is already source mapped. Probably doesn't work with stepping yet.
works with stepping and all that now, but not with other source maps yet.
Attachment #799856 - Attachment is obsolete: true
Blocks: 913665
Works with source mapped sources as well now.

https://tbpl.mozilla.org/?tree=Try&rev=f714822147e7
Attachment #800962 - Attachment is obsolete: true
Attachment #801780 - Flags: review?(past)
Pull request to the debugger docs repo for these protocol changes: https://github.com/jimblandy/DebuggerDocs/pull/16
Pull request for the minor source-map lib changes included in this patch: https://github.com/mozilla/source-map/pull/77
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.
Attachment #801780 - Flags: review?(past) → review+
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.
Whiteboard: [debugger-docs-needed] → [debugger-docs-needed][leave-open]
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).
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 :)
Depends on: 914930
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
Attachment #805709 - Flags: review?(vporof)
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.
Attachment #805709 - Flags: review?(vporof) → review+
https://hg.mozilla.org/integration/fx-team/rev/cc0aa7f1f41f
Whiteboard: [debugger-docs-needed][leave-open] → [fixed-in-fx-team]
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
Attachment #807008 - Flags: review+
(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
https://hg.mozilla.org/mozilla-central/rev/cc0aa7f1f41f
https://hg.mozilla.org/mozilla-central/rev/0a8d751016f9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Depends on: 918802
Depends on: 918797
Depends on: 918821
Add it as a dev note on release notes.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.