918797 - Trying to prettify html irrevocably loses the source until the page is refreshed

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Victor Porof [:vporof][:vp]]

STR:

1. Go to http://htmlpad.org/debugger/
2. Open debugger
3. Prettify the source

Error loading source:
SyntaxError: syntax error
Stack: SA__parseAST@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js:2421
resolve@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:43
resolve@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
resolve@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:118
then@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:43
resolve@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/commonjs/sdk/core/promise.js:185
fetch/streamListener.onStopRequest@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js:4062

Obviously, because it's HTML. Maybe we should do something about prettifying JS inside HTML as well, but in the meantime, I can never ever revert back to the actual source. The editor will forever display this error :(

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: dbg-prettify.patch

Comment 2

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

Comment on attachment 807773 [details] [diff] [review]
dbg-prettify.patch

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

r+ with comments addressed. Thanks!

::: browser/devtools/debugger/debugger-controller.js
@@ +1185,5 @@
>          DebuggerController.Parser.clearSource(aSource.url);
>  
>          if (this.activeThread.paused) {
>            // Update the stack frame list.
>            this.activeThread._clearFrames();

Aw crap, I forgot to publicize this method. Feel free to do it in this patch if you want. Otherwise I'll file a bug and [good-first-bug] it, so some contributor can learn how to get firefox built, and run tests, etc.

::: browser/devtools/debugger/debugger-panes.js
@@ +386,4 @@
>        }
>      };
> +    const printError = ([{ url }, error]) => {
> +      let msg = "Couldn't prettify source: " + url + "\n" + error;

Should use DevToolsUtils.safeErrorString to stringify the error.

I think we still need to have some kind of feedback for the user if we fail to pretty print, saying "go look in the console" isn't good enough. Perhaps we can invalidate the source cache, but still show the error. That way if they select a different source, then go back, it will be restored. Kind of meh, I guess.

This should just wait on bug 916180, so we can show the error, and then they can just toggle the button and we can show them normal source again.

@@ +1006,5 @@
> +   *         True if the source is likely javascript.
> +   */
> +  isJavaScript: function(aUrl, aContentType = "") {
> +    return /\.jsm?$/.test(this.trimUrlQuery(aUrl)) ||
> +           /javascript/.test(aContentType);

Nit: aContentType.contains("javascript") since you aren't using any regex, really.

::: browser/devtools/debugger/test/browser_dbg_pretty-print-05.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Make sure that clicking the pretty print button prettifies the source.

Update comment for this test.

::: browser/devtools/debugger/test/browser_dbg_pretty-print-06.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Make sure that clicking the pretty print button prettifies the source.

Update comment for this test.

::: browser/devtools/debugger/test/doc_pretty-print.html
@@ +1,2 @@
> +<!-- Any copyright is dedicated to the Public Domain.
> +     http://creativecommons.org/publicdomain/zero/1.0/ -->

Thanks

Comment 3

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #2)

Thanks for the review.

>
> Aw crap, I forgot to publicize this method. Feel free to do it in this patch
> if you want. Otherwise I'll file a bug and [good-first-bug] it, so some
> contributor can learn how to get firefox built, and run tests, etc.
> 

Meh.

> 
> I think we still need to have some kind of feedback for the user if we fail
> to pretty print, saying "go look in the console" isn't good enough. Perhaps
> we can invalidate the source cache, but still show the error. That way if
> they select a different source, then go back, it will be restored. Kind of
> meh, I guess.
> 

I think showing an entire stack trace in the source editor is a bit overkill. See the comment 0 for how this would look like. This makes the debugger seem incredibly broken.

We need a better way of displaying errors when they happen. Using the source editor is quite a gross hack.

> This should just wait on bug 916180, so we can show the error, and then they
> can just toggle the button and we can show them normal source again.
> 

Yeah, let's do that.

Comment 4

[Victor Porof [:vporof][:vp]]

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

Comment 5

[Victor Porof [:vporof][:vp]]

Try was green: https://tbpl.mozilla.org/?tree=Try&rev=af30a04a22ca

Comment 6

[Victor Porof [:vporof][:vp]]

...but the push had an orange. Potential fix: https://hg.mozilla.org/integration/fx-team/rev/7f8feafb0a44

Comment 7

[Victor Porof [:vporof][:vp]]

My fix in comment 6 was correct, but it evidenced an ever worse problem.

Amazing things are happening here. This is an ulterior try push, with the fix above applied: https://tbpl.mozilla.org/php/getParsedLog.php?id=28196483&tree=Try&full=1

It seems that, because the source has a type error in it, and hence it will never run, it will sometimes get gc'd before the target actually finishes navigating, so the debugger will never get to see it. From the screenshot in that log, it certainly looks that way.

This is non-deterministic and seems to only be happening on Windows 8 debug test slaves.

I think we should revisit what the test does. It just needs to throw some unparse-able text at the beautifier, which needs to respond with an error. Instead of trying to load an actual source with a type error, it's easier to make the backend lie about the source's text in order to pull an error from the parser.

What do you think Nick?

Comment 8

[Victor Porof [:vporof][:vp]]

I filed a prophylactic bug 919161.

Comment 9

[Victor Porof [:vporof][:vp]]

It happened on a OS X 10.6 debug slave as well, backed out: https://hg.mozilla.org/integration/fx-team/rev/a8adb2b75a14

Comment 10

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

Can we monkey punch the source actor from mochitests? If possible, that's what I would recommend; make its so pretty printing returns an error regardless of what you throw at it.

Otherwise, perhaps another global level function can reference the syntax error one, and this will keep it from getting GC'd? Gross and hacky, but if it is our only choice...

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #10)
> Can we monkey punch the source actor from mochitests?
> 

I don't know.

> Otherwise, perhaps another global level function can reference the syntax
> error one, and this will keep it from getting GC'd? Gross and hacky, but if
> it is our only choice...

I'll give this a shot. If not, then accurately testing this would be potentially complicated.

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #11)
> > Otherwise, perhaps another global level function can reference the syntax
> > error one, and this will keep it from getting GC'd? Gross and hacky, but if
> > it is our only choice...
> 

This doesn't seem to work. Having a type error in a source prevents *all* of it from being executed.

Comment 13

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #12)

Brain fart, I meant *Syntax Error* the whole time.

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: dbg-pretty-fix.patch

This is so incredibly gross... but it works!

Comment 15

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

Comment on attachment 808206 [details] [diff] [review]
dbg-pretty-fix.patch

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

I think this is pretty sub-optimal.

What if we did something like this (not tested):

gClient.request = (function (origRequest) {
  return function (packet, cb) {
    if (packet.type == "prettyPrint") {
      return executeSoon(() => cb({ error: "prettyPrintError" }));
    }
    return origRequest(packet, cb);
  };
}(gClient.request));

Comment 16

[Victor Porof [:vporof][:vp]]

Attachment: dbg-pretty-fix.patch

Thank you for not r+ing that. This approach looks and feels better.

Comment 17

[Victor Porof [:vporof][:vp]]

Eggcellent!

Comment 18

[Victor Porof [:vporof][:vp]]

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

Comment 19

[Carsten Book [:Tomcat]]

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