Closed Bug 1513014 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: ochameau, Assigned: jonco)

Details

Attachments

(1 file)

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.
Jon, I've been told that you are a good person to help me figure this out?
Flags: needinfo?(jcoppeard)
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)
Seems like Element() really is only needed for column and line numbers, we could just set those to default values for now.
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.
Flags: needinfo?(jcoppeard)

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

Flags: needinfo?(poirot.alex)

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";

Flags: needinfo?(poirot.alex)
Blocks: 1522042

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

Assignee: nobody → jcoppeard

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.

Attachment #9038605 - Flags: review?(bugs)
Attachment #9038605 - Flags: review?(bugs) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a68a2da104f3
Defer reporting errors while preloading until the request is actually used r=smaug
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

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

No longer blocks: 1522042
You need to log in before you can comment on or make changes to this bug.