Module loading errors should include source position information

RESOLVED FIXED in Firefox 57

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

a year ago
There are currently TODOs in HandleResolveFailure and HandleModuleNotFound for this.

Bug 1362097 will provide a way of getting this information.
Priority: -- → P2
(Assignee)

Updated

8 months ago
Assignee: nobody → jcoppeard
Summary: Errors thrown by HostResolveImportedModule should include source position information → Module loading errors should include source position information
(Assignee)

Comment 1

8 months ago
Created attachment 8900206 [details] [diff] [review]
bug1362098-import-export-errors

Report source position information for module resolution failures.

The patch adds line and column number properties to module import and export entry objects.  These are required for import entries but optional for export entries because we only use this information to report errors against indirect exports (direct exports are checked at parse time).

These are populated with information from the parser by ModuleBuilder.

They are used when we create an error object in Module.js to provide source information.
Attachment #8900206 - Flags: review?(till)
(Assignee)

Comment 2

8 months ago
Created attachment 8900208 [details] [diff] [review]
bug1362098-requested-modules

Provide source position information for a module's imports.

The patch adds a new RequestedModuleObject that describes an imported module including the module specifier string and the source line and column.  Previously just a string was used for this purpose.

This information is used by the DOM module loader.  Convenience JS APIs are provided for getting the properties of a RequestedModuleObject.
Attachment #8900208 - Flags: review?(till)
(Assignee)

Comment 3

8 months ago
Created attachment 8900210 [details] [diff] [review]
bug1362098-script-loader

Update the script loader to set source position information when handling module resolution failures, using the APIs added by the previous patch.
Attachment #8900210 - Flags: review?(bkelly)
(Assignee)

Comment 4

8 months ago
Created attachment 8900212 [details] [diff] [review]
bug1362098-tests

Add tests to exercise all of the above.
Attachment #8900212 - Flags: review?(bkelly)
(Assignee)

Updated

8 months ago
Duplicate of this bug: 1362097
Comment on attachment 8900206 [details] [diff] [review]
bug1362098-import-export-errors

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

r=me
Attachment #8900206 - Flags: review?(till) → review+
Comment on attachment 8900208 [details] [diff] [review]
bug1362098-requested-modules

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

r=me, with or without comments addressed.

::: js/src/builtin/ModuleObject.cpp
@@ +283,5 @@
> +
> +    if (!DefinePropertiesAndFunctions(cx, proto, protoAccessors, nullptr))
> +        return false;
> +
> +    global->setReservedSlot(REQUESTED_MODULE_PROTO, ObjectValue(*proto));

Use initReservedSlot (or initFixedSlot) here to get the isUndefined assert, maybe?

@@ +303,5 @@
> +
> +    RootedRequestedModuleObject self(cx, &obj->as<RequestedModuleObject>());
> +    self->initReservedSlot(ModuleSpecifierSlot, StringValue(moduleSpecifier));
> +    self->initReservedSlot(LineNumberSlot, Int32Value(lineNumber));
> +    self->initReservedSlot(ColumnNumberSlot, Int32Value(columnNumber));

I'd use *FixedSlot for uses like this. However, using *ReservedSlot is certainly more in line with the rest of the Modules code, and it doesn't make much of a difference, I guess.
Attachment #8900208 - Flags: review?(till) → review+

Comment 8

8 months ago
Comment on attachment 8900210 [details] [diff] [review]
bug1362098-script-loader

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

::: dom/script/ScriptLoader.cpp
@@ +675,5 @@
>      ModuleScript* ms = aRequest->mModuleScript;
>      nsCOMPtr<nsIURI> uri = ResolveModuleSpecifier(ms, specifier);
>      if (!uri) {
> +      uint32_t lineNumber;
> +      uint32_t columnNumber;

Please init these to some default value for sanity's sake.
Attachment #8900210 - Flags: review?(bkelly) → review+

Updated

8 months ago
Attachment #8900212 - Flags: review?(bkelly) → review+

Comment 9

8 months ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1d01918122f
Report source position information for module export resolution failures r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/a953b8493339
Add source position to requested module information r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/514a186794df
Report source position for module specfier resolution failure r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d84e2c30a41
Add tests for module error reporting r=bkelly

Comment 10

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e1d01918122f
https://hg.mozilla.org/mozilla-central/rev/a953b8493339
https://hg.mozilla.org/mozilla-central/rev/514a186794df
https://hg.mozilla.org/mozilla-central/rev/2d84e2c30a41
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1439416
You need to log in before you can comment on or make changes to this bug.