Some ES module import failures aren't logged because of null Element
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox66 | --- | fixed |
People
(Reporter: ochameau, Assigned: jonco)
Details
Attachments
(1 file)
|
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
|
Reporter | |
Comment 1•6 years ago
|
||
Jon, I've been told that you are a good person to help me figure this out?
|
|
Reporter | |
Comment 2•6 years ago
|
||
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•6 years ago
|
||
Seems like Element() really is only needed for column and line numbers, we could just set those to default values for now.
|
Assignee | |
Comment 4•6 years ago
|
||
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.
|
|
Assignee | |
Comment 5•6 years ago
|
||
Hi, this check has been removed in a recent change. Is the problem still happening?
|
|
Reporter | |
Comment 6•6 years ago
|
||
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";
|
|
Assignee | |
Comment 7•6 years ago
|
||
Thanks for checking. I've reproduced the problem locally and I'm investigating.
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 8•6 years ago
|
||
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.
|
||
Updated•6 years ago
|
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
|
||
Comment 10•6 years ago
|
||
| bugherder | ||
|
||
Comment 11•6 years ago
|
||
Thanks jonco, this reminds me of the issue we found with breakpoints and pre-loading this summer.
Description
•