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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Yury Delendik (:yury)]

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.

Comment 1

[Yury Delendik (:yury)]

Attachment: Bug 1431864 - Use Response.url as WebAssembly module URL.

Review commit: https://reviewboard.mozilla.org/r/214694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214694/

Comment 2

[Yury Delendik (:yury)]

Comment on attachment 8944530 [details]
Bug 1431864 - Use Response.url as WebAssembly module URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214694/diff/1-2/

Comment 3

[Yury Delendik (:yury)]

Comment on attachment 8944530 [details]
Bug 1431864 - Use Response.url as WebAssembly module URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214694/diff/2-3/

Comment 4

[Yury Delendik (:yury)]

Comment on attachment 8944530 [details]
Bug 1431864 - Use Response.url as WebAssembly module URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214694/diff/3-4/

Comment 5

[Luke Wagner [:luke]]

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 6

[Yury Delendik (:yury)]

Comment on attachment 8944530 [details]
Bug 1431864 - Use Response.url as WebAssembly module URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214694/diff/4-5/

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

[Luke Wagner [:luke]]

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

Comment 9

[Yury Delendik (:yury)]

Comment on attachment 8944530 [details]
Bug 1431864 - Use Response.url as WebAssembly module URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214694/diff/5-6/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53c5a199232a
Use Response.url as WebAssembly module URL. r=bkelly,luke

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/53c5a199232a