The default bug view has changed. See this FAQ.

An option to deobfuscate javascripts in the debugger

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Salar Khalilzadeh, Assigned: fitzgen)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 27
Other
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 27+)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
Related to bug 670002

Comment 2

5 years ago
This is likely a dupe to one of the bugs linked to from
https://wiki.mozilla.org/DevTools/Features/SourceMap
(Reporter)

Comment 3

5 years ago
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.

Comment 6

5 years ago
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.

Updated

5 years ago
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

Updated

5 years ago
Duplicate of this bug: 767177
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
Also see bug 640631
And https://addons.mozilla.org/en-US/firefox/addon/javascript-deminifier/
(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 :(
Duplicate of this bug: 873347
Assignee: nobody → nfitzgerald
Priority: P3 → P2
Depends on: 908913
Status: NEW → ASSIGNED
Whiteboard: [debugger-docs-needed]
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.
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.
Attachment #799856 - Attachment is obsolete: true

Updated

4 years ago
Blocks: 913665
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
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 :)
Landed part 1: https://hg.mozilla.org/integration/fx-team/rev/9929cf38b486
https://hg.mozilla.org/mozilla-central/rev/9929cf38b486
Flags: in-testsuite+
Depends on: 914930
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
Attachment #805709 - Flags: review?(vporof)
Blocks: 917072
Blocks: 815280
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]
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
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27

Updated

4 years ago
Depends on: 918802
Depends on: 918797
Depends on: 918821
relnote-firefox: --- → ?
Add it as a dev note on release notes.

Updated

3 years ago
relnote-firefox: ? → 27+
You need to log in before you can comment on or make changes to this bug.