906889 - odd stoppage on reloading page with debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

STR:

1. Open http://people.mozilla.com/~vladimir/misc/hello_gl.html
2. Open up debugger (C-Shift-K, click Debugger)
3. Search for #WindowSize
4. Set a breakpoint on the Browser.setCanvasSize line
5. Make sure "Pause on exceptions" is OFF
6. Reload page

You're now at an odd state: the "play" button isn't highlighted (which should mean "running"), but the page is not running.

Hitting "play" ends up taking you to the breakpoint set earlier and stopping there.  Hitting play again continues to the end of the scripts.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

It doesn't seem to pause for me, on the latest nightly. I get a bunch of errors in the browser console (CCing Nick as they are sourcemap-related), but execution doesn't pause with the debugger either open or closed:

[16:35:33.052] JSON.parse: unexpected character:
SourceMapConsumer@resource://gre/modules/devtools/SourceMap.jsm:67
TS__fetchSourceMap/promise<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js:3503
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:3776
 @ resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js:3826

I can only see a textarea and some input controls, is this expected?

Comment 2

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

I'm not seeing anything in the browser console other than:

[08:57:14.227] SyntaxError: Using //@ to indicate source map URL pragmas is deprecated. Use //# instead @ http://people.mozilla.com/~vladimir/misc/hello_gl.html:4171

Comment 3

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

Furthermore, I hit the breakpoint on reload and everything seems to be working as expected.

Comment 4

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

The debugger broke for me with the following error:

[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIChannel.contentCharset]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/script.js :: fetch/streamListener.onStopRequest :: line 4148"  data: no]

Nightly.

The debugger was empty the first time I opened it, and then the whole toolbox refused to open on subsequent attempts.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

I have a fix for this that I will upload shortly.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make the debugger more resilient in the presence of a bogus source map

This patch fixes both the symptoms in this bug as well as bug 906893. We weren't handling bogus or non-existent source maps properly. I'm not sure about testing it though. Any suggestions Nick?

Comment 7

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

Comment on attachment 815889 [details] [diff] [review]
Make the debugger more resilient in the presence of a bogus source map

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

::: toolkit/devtools/server/actors/script.js
@@ +786,3 @@
>          generatedLocation));
> +      if (originalLocation) {
> +        const { url } = originalLocation;

Setting url inside this block only works because of a bug in spidermonkey. Const is supposed to have block scope like let, but in currently SM, has function scope like var.

const url = originalLocation ? originalLocation.url : null;

And elsewhere.

However, if ThreadSources#getOriginalLocation should handle errors for us. There shouldn't ever be a time when it doesn't return an object of the form { url, line, column }. Can you move the fix to the source of the problem (inside getOriginalLocation) instead of patching everywhere that calls it? I suspect adding

  .then(null, error => {
    DevToolsUtils.reportException(error);
    return { url: null, line: null, column: null };
  });

to the promise in the branch where we do have a source map should suffice, right?

> This patch fixes both the symptoms in this bug as well as bug 906893. We
> weren't handling bogus or non-existent source maps properly. I'm not sure
> about testing it though. Any suggestions Nick?

In the case of non-existent source maps, we should be returning the url/line/column that was passed in, no? Why would that ever not be the case?

In the case of bogus source maps, that should usually be handled when we create a SourceMapConsumer for the source map. Are we not cleaning up after ourselves enough? http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#3800

However, now that we lazily parse the mappings in a source map, there is the case where we first parse the mappings and it could be triggered from a call to getOriginalLocation. However, in this case, I think that adding the above error handler should still be enough. Maybe in that case we want to invalidate the source map, and just debug the generated source. However that would require telling the client about new sources, which might be weird. Thoughts?

In either case, I would just find out in what way the source maps were bogus, and create a minimal test case with the same kind of invalidity in a smaller source map, and verify that we don't choke on it.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make the debugger more resilient in the presence of a bogus source map v2

You are right about handling the problem inside getOriginalLocation, this works fine, too. When I mentioned non-existent source maps, I was referring to the server returning a 404 for the source map, not a source without a source map pragma.

Testing a 404 seems straightforward, but the problem is that we log the error to the console and xpcshell treats this as a failure. I can't think of a way to overcome this without making the test totally fabricated and useless. Any ideas?

Comment 9

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

(In reply to Panos Astithas [:past] from comment #8)
> Created attachment 816573 [details] [diff] [review]
> Make the debugger more resilient in the presence of a bogus source map v2
> 
> You are right about handling the problem inside getOriginalLocation, this
> works fine, too. When I mentioned non-existent source maps, I was referring
> to the server returning a 404 for the source map, not a source without a
> source map pragma.

Ah, ok.

> 
> Testing a 404 seems straightforward, but the problem is that we log the
> error to the console and xpcshell treats this as a failure. I can't think of
> a way to overcome this without making the test totally fabricated and
> useless. Any ideas?

Can we add a global to stop the fail-on-error-log behavior for specific tests? Then in set up phase you could do something like `FAIL_ON_ERRORS = false;` and just be absolutely sure we set it back to true when the test completes. Assuming that different xpcshell tests can run in the same env; if that's false we don't need to worry about flipping it back, right? This is a little harder in xpcshell tests since we don't have `registerCleanUpFunction` like mochitests have, but do-able, I think.

Comment 10

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

Comment on attachment 816573 [details] [diff] [review]
Make the debugger more resilient in the presence of a bogus source map v2

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

LGTM, just need test(s)

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make the debugger more resilient in the presence of a bogus source map v3

Now with a test. I couldn't get breakage to occur with an xpcshell test, but after a lot of hair-pulling I managed to create a browser mochitest that fails without the fix.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=42c4c73e6779

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make the debugger more resilient in the presence of a bogus source map v4

The previous patch and test worked, but it emitted an error in the console that was showing up in TBPL logs, potentially causing confusion. I spent a lot of time trying to find a nice and clean way to hide it, but in the end I decided to use a flag to disable this logging. I'm not concerned about the perf impact, since this is a code path only exercised in case of bogus source maps, and even then the extra overhead should be minimal.

Try run to confirm logs are clean: https://tbpl.mozilla.org/?tree=Try&rev=d924e083da75

Comment 14

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

Comment on attachment 820948 [details] [diff] [review]
Make the debugger more resilient in the presence of a bogus source map v4

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

::: browser/devtools/debugger/test/browser_dbg_source-maps-04.js
@@ +64,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function reloadPage() {

Can we use reloadActiveTab from head.js here?

@@ +70,5 @@
> +  gDebugger.gClient.activeTab.reload();
> +  return loaded.then(() => ok(true, "Page was reloaded and execution resumed."));
> +}
> +
> +function enablePauseOnExceptions() {

Nit: Could you re-arrange the function definitions into the same order as they are in the promise .then chain? I think it is a little easier to follow the test when this is the case.

::: browser/devtools/debugger/test/code_math_bogus_map.min.js
@@ +1,1 @@
> +// This comment is a hack to work around an Orion bug that doesn't update the caret position on pause.

Orion? WTF is Orion? ;)

(Can we remove this now that CM landed?)

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make the debugger more resilient in the presence of a bogus source map v5

(In reply to Nick Fitzgerald [:fitzgen] from comment #14)
> > +function reloadPage() {
> 
> Can we use reloadActiveTab from head.js here?

Unfortunately not, because I need to wait for two events in this case. We should definitely add that capability to reloadActiveTab though. I have addressed the rest of your comments and added a forgotten license block.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 17

[Carsten Book [:Tomcat]]

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