Closed Bug 918797 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(2 files, 1 obsolete file)

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 :(
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 762761
Attachment #807773 - Flags: review?(nfitzgerald)
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
Attachment #807773 - Flags: review?(nfitzgerald) → review+
(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.
...but the push had an orange. Potential fix: https://hg.mozilla.org/integration/fx-team/rev/7f8feafb0a44
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?
Flags: needinfo?(nfitzgerald)
I filed a prophylactic bug 919161.
It happened on a OS X 10.6 debug slave as well, backed out: https://hg.mozilla.org/integration/fx-team/rev/a8adb2b75a14
Whiteboard: [fixed-in-fx-team]
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...
Flags: needinfo?(nfitzgerald)
(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.
(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.
(In reply to Victor Porof [:vp] from comment #12)

Brain fart, I meant *Syntax Error* the whole time.
Attached patch dbg-pretty-fix.patch (obsolete) — Splinter Review
This is so incredibly gross... but it works!
Attachment #808206 - Flags: review?(nfitzgerald)
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));
Attachment #808206 - Flags: review?(nfitzgerald)
Thank you for not r+ing that. This approach looks and feels better.
Attachment #808252 - Flags: review?(nfitzgerald)
Attachment #808206 - Attachment is obsolete: true
Attachment #808252 - Flags: review?(nfitzgerald) → review+
Eggcellent!
Relanded: https://hg.mozilla.org/integration/fx-team/rev/a9fd089aeefb
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a9fd089aeefb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.