Baldr: use Reponse.url in WebAssembly.Instance for display in DevTools

RESOLVED FIXED in Firefox 60

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a month ago
16 days ago

People

(Reporter: yury, Assigned: yury)

Tracking

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a month ago
After implementation of WebAssembly.compileStreaming/instantiateStreaming (bug 1347644), we can display "real" URL of the WebAssembly bytecode in the DevTools. Currently we generate a fake URL based on script caller information. The Response object contains the url property we can use instead.

Also, the WebAssemblu source maps specification (at https://github.com/WebAssembly/design/pull/1051) requires usage of this information if sourceMappingURL is relative.
(Assignee)

Updated

a month ago
Summary: Baldr: use Reponse.url to WebAssembly.Instance for use in DevTools → Baldr: use Reponse.url in WebAssembly.Instance for display in DevTools
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

a month ago
mozreview-review
Comment on attachment 8944530 [details]
Bug 1431864 - Use Response.url as WebAssembly module URL.

https://reviewboard.mozilla.org/r/214692/#review220702

Thanks and looks pretty good, mostly I just have nits.  Could you also flag bkelly for review on the FetchUtil bits since I'm not a module peer of that module.

::: dom/fetch/FetchUtil.cpp:573
(Diff revision 4)
>  
> +  switch (aMimeType) {
> +    case JS::MimeType::Wasm:
> +      nsAutoString url;
> +      response->GetUrl(url);
> +      ErrorResult result;

nit: \n before ErrorResult decl

::: dom/fetch/FetchUtil.cpp:577
(Diff revision 4)
> +      response->GetUrl(url);
> +      ErrorResult result;
> +      nsCString sourceMapUrl;
> +      response->GetInternalHeaders()->Get(NS_LITERAL_CSTRING("SourceMap"), sourceMapUrl, result);
> +      if (NS_WARN_IF(result.Failed())) {
> +          return ThrowException(aCx, JSMSG_ERROR_CONSUMING_RESPONSE);

nit: two space indent

::: dom/fetch/FetchUtil.cpp:581
(Diff revision 4)
> +      if (NS_WARN_IF(result.Failed())) {
> +          return ThrowException(aCx, JSMSG_ERROR_CONSUMING_RESPONSE);
> +      }
> +      NS_ConvertUTF16toUTF8 urlUTF8(url);
> +      aConsumer->noteResponseURLs(urlUTF8.get(),
> +                                  !sourceMapUrl.IsVoid() ? sourceMapUrl.get() : nullptr);

nit: how about remove ! and switch ternary case order

::: js/src/jsapi.h:4596
(Diff revision 4)
>      // contract described above.
>      enum CloseReason { EndOfFile, Error };
>      virtual void streamClosed(CloseReason reason) = 0;
> +
> +    // Provides optional stream attributes such as base or source mapping URLs.
> +    // Necessarily called before consumeChunk() or streamClosed().

Could you add "The caller retains ownership of the given strings" and change 'sourceMapUrl' to 'maybeSourceMapUrl'?

::: js/src/shell/js.cpp:5859
(Diff revision 4)
> +            Rooted<JSFlatString*> flat(cx, value.toString()->ensureFlat(cx));
> +            if (!flat)
> +                return false;
> +            if (!chars.initLengthUninitialized(JS::GetDeflatedUTF8StringLength(flat) + 1))
> +                return false;
> +            JS::DeflateStringToUTF8Buffer(flat, mozilla::RangedPtr<char>(chars.begin(), chars.length()));

This is all somewhat pedantic because we're in js.cpp dealing with shell-only inputs, but I think we don't want to deflate to UTF8 here, but rather call encodeURI().  Alternatively, we can be arbitrary here and throw an error if !hasLatin1Chars() and then you don't need to make a copy, you can just pass pass latin1Chars() directly to noteResponseURLs().

Also, as a nit: can you use a static function instead of nested lambda.

::: js/src/vm/MutexIDs.h:52
(Diff revision 4)
>    _(WasmModuleTieringLock,       500) \
>    _(WasmCompileTaskState,        500) \
>    _(WasmCodeStreamEnd,           500) \
>    _(WasmTailBytesPtr,            500) \
>    _(WasmStreamStatus,            500) \
> +  _(WasmResponseHints,           500) \

I think this shouldn't be necessary anymore

::: js/src/wasm/WasmDebug.cpp:687
(Diff revision 4)
>          result.set(str);
> -        break;
> +        return true;
> +    }
> +
> +    // Check presence of "SourceMap:" HTTP response header.
> +    auto sourceMapURL = metadata().sourceMapURL.get();

nit: since we're doing a reinterpret_cast later, it'd be nice to see what the type of sourceMapURL is here.

::: js/src/wasm/WasmJS.cpp:2203
(Diff revision 4)
> -    const SharedCompileArgs      compileArgs_;
>      const bool                   instantiate_;
>      const PersistentRootedObject importObj_;
>  
>      // Mutated on a stream thread (consumeChunk() and streamClosed()):
> +    const MutableCompileArgs      compileArgs_;

nit: re-align 'compileArgs_'

::: js/src/wasm/WasmJS.cpp:2376
(Diff revision 4)
>              break;
>          }
>          MOZ_CRASH("unreachable");
>      }
>  
> +    void noteResponseURLs(const char* url, const char* sourceMapUrl) override {

nit: could you move this function before the "// Called on a stream thread" comment and add a "// Called on some thread before consumeChunk() or streamClosed()" comment (with \n before/after).

::: js/src/wasm/WasmJS.cpp:2386
(Diff revision 4)
> +    }
> +
>      // Called on a helper thread:
>  
>      void execute() override {
> -        module_ = CompileStreaming(*compileArgs_, envBytes_, codeBytes_, exclusiveCodeStreamEnd_,
> +        module_ = CompileStreaming(*const_cast<const CompileArgs *>(compileArgs_.get()),

If this is going from a 'CompileArgs*' to a 'const CompileArgs*', it seems like a const_cast wouldn't be necessary.
Comment hidden (mozreview-request)

Comment 7

a month ago
mozreview-review
Comment on attachment 8944530 [details]
Bug 1431864 - Use Response.url as WebAssembly module URL.

https://reviewboard.mozilla.org/r/214694/#review221014

I only reviewed FetchUtils.cpp.

::: dom/fetch/FetchUtil.cpp:578
(Diff revision 5)
> +
> +      ErrorResult result;
> +      nsCString sourceMapUrl;
> +      response->GetInternalHeaders()->Get(NS_LITERAL_CSTRING("SourceMap"), sourceMapUrl, result);
> +      if (NS_WARN_IF(result.Failed())) {
> +        return ThrowException(aCx, JSMSG_ERROR_CONSUMING_RESPONSE);

You need to call `result.SuppressException()` or use `IgnoredErrorResult` which auto-suppresses.  Otherwise you might get an assertion if there is a js exception in the ErrorResult and its not consumed.
Attachment #8944530 - Flags: review?(bkelly) → review+

Comment 8

a month ago
mozreview-review
Comment on attachment 8944530 [details]
Bug 1431864 - Use Response.url as WebAssembly module URL.

https://reviewboard.mozilla.org/r/214694/#review221042

Thanks!

::: js/src/jsapi.h:4602
(Diff revision 5)
>      virtual void streamClosed(CloseReason reason) = 0;
> +
> +    // Provides optional stream attributes such as base or source mapping URLs.
> +    // Necessarily called before consumeChunk() or streamClosed(). The caller
> +    // retains ownership of the given strings.
> +    virtual void noteResponseURLs(const char* url, const char* maybeSourceMapUrl) = 0;

Oops, I hadn't notied before that 'url' is also option.  Could you rename also to 'maybeBaseUrl'?

::: js/src/wasm/WasmCode.h:413
(Diff revision 5)
>      TableDescVector       tables;
>      NameInBytecodeVector  funcNames;
>      CustomSectionVector   customSections;
>      CacheableChars        filename;
> +    CacheableChars        baseURL;
> +    CacheableChars        sourceMapURL;

Could you serialize baseURL and sourceMapURL?  Just search for and copypasta 'filename' in the 4 adjacent functions starting with Metadata::serializedSize() in WasmCode.cpp.

::: js/src/wasm/WasmDebug.cpp:689
(Diff revision 5)
> +    }
> +
> +    // Check presence of "SourceMap:" HTTP response header.
> +    char* sourceMapURL = metadata().sourceMapURL.get();
> +    if (sourceMapURL && strlen(sourceMapURL)) {
> +        UTF8Chars utf8Chars(const_cast<const char*>(sourceMapURL), strlen(sourceMapURL));

nit: I don't think you need an explicit const_cast to add constness.

::: js/src/wasm/WasmJS.cpp:2203
(Diff revision 5)
> -    const SharedCompileArgs      compileArgs_;
>      const bool                   instantiate_;
>      const PersistentRootedObject importObj_;
>  
>      // Mutated on a stream thread (consumeChunk() and streamClosed()):
> +    const MutableCompileArgs     compileArgs_;

nit: could you put compileArgs_ back to the "Immutable" section but extend the comment to say "Immutable during streaming"?

::: js/src/wasm/WasmJS.cpp:2388
(Diff revision 5)
>      }
>  
>      // Called on a helper thread:
>  
>      void execute() override {
> -        module_ = CompileStreaming(*compileArgs_, envBytes_, codeBytes_, exclusiveCodeStreamEnd_,
> +        module_ = CompileStreaming(*compileArgs_.get(), envBytes_, codeBytes_, exclusiveCodeStreamEnd_,

nit: I don't think the .get() is necessary; this line can stay unchanged
Attachment #8944530 - Flags: review?(luke) → review+
Comment hidden (mozreview-request)

Comment 10

29 days ago
Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53c5a199232a
Use Response.url as WebAssembly module URL. r=bkelly,luke

Comment 11

29 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53c5a199232a
Status: NEW → RESOLVED
Last Resolved: 29 days ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
status-firefox59: affected → ---
You need to log in before you can comment on or make changes to this bug.