1513014 - Some ES module import failures aren't logged because of null Element

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alexandre Poirot [:ochameau]]

While trying to make the debugger use native ES modules to load its frontend modules, I hit many silent errors.

While doing this patch, some import were wrong, but no error was logged anywhere.

I looked into that and figured out that it was failing because of this check:
https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/dom/script/ScriptLoader.cpp#2910
  if (!aRequest->Element()) {
    return;
  }

Here is a patch to reproduce this issue:
https://hg.mozilla.org/try/rev/a00266fd4b6f6584938539859f2fa1c87aa19b88
You will have to apply the debugger refactoring to es modules first:
 https://hg.mozilla.org/try/rev/3318e8e424df
Both patches are based on top of this changeset:
 https://hg.mozilla.org/try/rev/53fd96ca5aa4

The STR is simple, open the browser console first and then the debugger against any webpage. You will see in the browser console only the errors for the requests which had an Element being set.

Comment 1

[Alexandre Poirot [:ochameau]]

Jon, I've been told that you are a good person to help me figure this out?

Comment 2

[Alexandre Poirot [:ochameau]]

Note that the setup is complex and isn't pure content at all.

In involves a chrome document: chrome://devtools/content/debugger/new/index.html
Which lives here: https://hg.mozilla.org/try/file/3318e8e424df/devtools/client/debugger/new/index.html#l37
and loads a main es module like this:
 <script type="module" src="resource://devtools/client/debugger/new/src/main.js"></script>
Then it loads tons of es modules that comes from devtools/client/debugger/new/src/ folder,
unfortunately, it is utterly complex because these files aren't straight JS, they include JSX syntax (to ease working with React),
these files are processed by a nodejs script to be converted to pure JS/Es module. But this build step should not have anything to do with this issue. It just makes it harder to understand the whole debugger setup.

In the demo changeset, I introduced a voluntary error in this file:
  https://hg.mozilla.org/try/file/a00266fd4b6f/devtools/client/debugger/new/src/utils/bootstrap.js#l22
  import * as defaultActions from "../actionsxxx";
Which I never saw correctly logged in the browser console.
This file is loaded from this URL:
  resource://devtools/client/debugger/new/src/utils/bootstrap.js
(in case you want to see what the nodejs script builds out of the source file)

Comment 3

[Tom S]

Seems like Element() really is only needed for column and line numbers, we could just set those to default values for now.

Comment 4

[Jon Coppeard (:jonco)]

That check will be removed by patch 8 in bug 1342012.

I'm a bit confused why this is happening though because all requests should have an element set at this point, except preload requests.

Comment 5

[Jon Coppeard (:jonco)]

Hi, this check has been removed in a recent change. Is the problem still happening?

Comment 6

[Alexandre Poirot [:ochameau]]

Sorry it took me so long to get back to you, but yes, the issue still occurs and makes it really hard to debug ES Modules for the debugger.

STR is the same: open the browser console, then open the debugger against any page => no error logged about the broken import path.

Here is an updated patch for debugger based on es-modules:
https://hg.mozilla.org/try/rev/7b4d1a89340d8464bef3cdda615f0664bae69880
This is based on top of https://hg.mozilla.org/mozilla-central/log/e43944736829dc575bb12821f0e28bf1bc02c285

This patch doesn't include any error, you have to introduce one yourself. Here is an example:
diff --git a/devtools/client/debugger/new/src/utils/bootstrap.js b/devtools/client/debugger/new/src/utils/bootstrap.js
index d3cdc7d9609e..efbeef9b7c3d 100644
--- a/devtools/client/debugger/new/src/utils/bootstrap.js
+++ b/devtools/client/debugger/new/src/utils/bootstrap.js
@@ -19,7 +19,7 @@ import reducers from "../reducers";
import * as selectors from "../selectors";
import App from "../components/App";
import { asyncStore, prefs } from "./prefs";
-import * as defaultActions from "../actions";
+import * as defaultActions from "../actionxxxs";

import { Provider } from "react-redux";

Comment 7

[Jon Coppeard (:jonco)]

Thanks for checking. I've reproduced the problem locally and I'm investigating.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1513014-defer-reporting-preload-errors

The problem is that when we preload scripts, we don't know whether they will actually end up getting loaded, so we don't report errors for them. Then, if they are later used the error information has been lost.

The patch records unreported errors for preloaded scripts and and reports them at the point we decide to use the preload request.

This does generate a whole slew of errors for this particular case, but this is better than what we have now.

Using ES6 modules for the debugger is a pretty good test case and exposed a couple of places where ModuleLoadRequest methods can get called on a previously cancelled request. In debug builds this asserts when we call SetReady(). I added checks for these cases.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a68a2da104f3
Defer reporting errors while preloading until the request is actually used r=smaug

Comment 10

[Narcis Beleuzu [:NarcisB]]

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

Comment 11

[Jason Laster [:jlast]]

Thanks jonco, this reminds me of the issue we found with breakpoints and pre-loading this summer.