1355263 - [wasm] different wasm scripts might have non-unique source urls

Assignee

Description

•

7 years ago

In wasm module metadata we save filename, which is a filename of the script that instantiated the module. The generate source URL/filename we just append " > wasm" suffix to the filename for the debugger. It will be nice to generate somewhat unique id/filename for the modules.

For example:

function initWasm(s) { return new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(s))); }
o1 = initWasm('(module (func) (export "" 0))');
o2 = initWasm('(module (func) (func) (export "" 1))');

will report to the debugger that all sources have url: "test.js > wasm".

And the next example:

var g = newGlobal();
g.eval(`
function initWasm(s) { return new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(s))); }
o1 = initWasm('(module (func) (export "" 0))');
o2 = initWasm('(module (func) (func) (export "" 1))');
`);
function isWasm(script) { return script.format === "wasm"; }
var dbg = new Debugger(g);
var foundScripts = dbg.findScripts().filter(isWasm);
print(foundScripts[0].source.url);
print(foundScripts[1].source.url);

will print "test.js line 2 > eval > wasm" twice.

The debugger ignores duplicate script with the same URL and displays only one of them.

Comment hidden (mozreview-request)

The URLs will have the following format:
   wasm: [<uri-econded-filename-of-host> ":"] <SHA1-UUID>

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

Yury Delendik (:yury)

Assignee

Updated

•

7 years ago

Assignee: nobody → ydelendik

Blocks: wasm-tools

Yury Delendik (:yury)

Assignee

Updated

•

7 years ago

Attachment #8856759 - Flags: review?(luke)

Luke Wagner [:luke]

Comment 2

•

7 years ago

mozreview-review

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

https://reviewboard.mozilla.org/r/128686/#review131394

::: js/src/wasm/WasmCode.cpp:400
(Diff revision 1)
>  CacheableChars::sizeOfExcludingThis(MallocSizeOf mallocSizeOf) const
>  {
>      return mallocSizeOf(get());
>  }
>  
> +ModuleUUID::ModuleUUID() : data() {

nit: { on new line

::: js/src/wasm/WasmCode.cpp:416
(Diff revision 1)
> +
> +    /* put in the variant and version bits */
> +    time_hi_and_version &= 0x0FFF;
> +    time_hi_and_version |= (ModuleUUID::UUIDVersion << 12);
> +    clock_seq_hi_and_reserved &= 0x3F;
> +    clock_seq_hi_and_reserved |= 0x80;

What's.... all this stuff?  Is the SHA1 not sufficient?

Comment hidden (mozreview-request)

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

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

Yury Delendik (:yury)

Assignee

Comment 4

•

7 years ago

mozreview-review-reply

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

https://reviewboard.mozilla.org/r/128686/#review131394

> What's.... all this stuff?  Is the SHA1 not sufficient?

Valid UUID must have version and variant bits set (per RFC 4122). I added a comment about that.

Luke Wagner [:luke]

Comment 5

•

7 years ago

But it seems like we don't have a proper UUID since the bits of the UUID are uniquely determined by the bytecode contents.  Looking at Chrome, it seem they just have an 8-character hash; that would perhaps make this look less bulky in the UI, perhaps we could just XOR-fold the SHA1 down to something shorter?  Also, Chrome seems to use "wasm://"  not "wasm:" as the prefix.

Luke Wagner [:luke]

Comment 6

•

7 years ago

mozreview-review

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

https://reviewboard.mozilla.org/r/128686/#review131462

Sorry for the delay! After our last chat this dropped off my radar.  One request significant change requested and then I promise I'll re-review with haste.

::: js/src/wasm/WasmCode.h:425
(Diff revision 2)
> +            uint32_t  time_low;
> +            uint16_t  time_mid;
> +            uint16_t  time_hi_and_version;
> +            uint8_t   clock_seq_hi_and_reserved;
> +            uint8_t   clock_seq_low;
> +            uint8_t   node[6];

Since there's no requirement for having an RFC 4122-compliant UUID, I think I'd rather just use a plain char[8] array. This will feel less magical to people modifying the code in the future making it easier to refactor etc.

::: js/src/wasm/WasmCode.cpp:1155
(Diff revision 2)
> +    // - UUID based on SHA1 hash of the module bytes.
> +    js::StringBuffer result(cx);
> +    if (!result.append("wasm:"))
> +        return nullptr;
> +    js::StringBuffer filenamePrefix(cx);
> +    if (EncodeURI(filenamePrefix, metadata().filename.get(), strlen(metadata().filename.get())) &&

filename can be null

::: js/src/wasm/WasmCode.cpp:1156
(Diff revision 2)
> +    js::StringBuffer result(cx);
> +    if (!result.append("wasm:"))
> +        return nullptr;
> +    js::StringBuffer filenamePrefix(cx);
> +    if (EncodeURI(filenamePrefix, metadata().filename.get(), strlen(metadata().filename.get())) &&
> +        !(result.append(filenamePrefix.finishString()) && result.append(":")))

Also, could you break this fat conditional into two:
  if (!EncodeURI(...))
     return null
  if (!result.append(...))
     return null

Attachment #8856759 - Flags: review?(luke)

Comment hidden (mozreview-request)

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

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

Yury Delendik (:yury)

Assignee

Comment 8

•

7 years ago

mozreview-review-reply

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

https://reviewboard.mozilla.org/r/128686/#review131462

> Since there's no requirement for having an RFC 4122-compliant UUID, I think I'd rather just use a plain char[8] array. This will feel less magical to people modifying the code in the future making it easier to refactor etc.

Replaced by 64-bit hash (first 8 bytes of the SHA1)

> Also, could you break this fat conditional into two:
>   if (!EncodeURI(...))
>      return null
>   if (!result.append(...))
>      return null

Also improved handling of the bad URI.

Luke Wagner [:luke]

Comment 9

•

7 years ago

mozreview-review

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

https://reviewboard.mozilla.org/r/128686/#review135062

Thanks!

::: js/src/wasm/WasmCode.cpp:1089
(Diff revision 3)
> +        // EncodeURI returns false due to invalid chars or OOM -- fail only
> +        // during OOM.
> +        if (!uriEncoded && !cx->isExceptionPending())
> +            return nullptr;
> +        if (!uriEncoded)
> +            cx->clearPendingException(); // ignore invalid URI

Could the logic be:
  if (!EncodeURI(...)) {
     if (!cx->isExceptionPending())
         return nullptr;
     cx->clearPendingException();
  }
  if (!results.append ...)
     return nullptr

::: js/src/wasm/WasmCode.cpp:1096
(Diff revision 3)
> +            return nullptr;
> +    }
> +
> +    const ModuleHash& hash = metadata().hash;
> +    char* buf = JS_smprintf("%02x%02x%02x%02x%02x%02x%02x%02x",
> +                            hash[0], hash[1], hash[2], hash[3], hash[4], hash[5], hash[6], hash[7]);

Can you static_assert(sizeof(hash) == 8) and use UniqueChars to own 'buf'?

OR, better yet: you could write a for loop over the bytes (ranged for works on arrays, it seems) and use StringBuffer.append(char) to write your own hex, nibble at a time... that's pretty easy and saves a malloc ;)

::: js/src/wasm/WasmGenerator.cpp:1102
(Diff revision 3)
>      env_->elemSegments.back().elemCodeRangeIndices = Move(codeRangeIndices);
>      return true;
>  }
>  
> +void
> +ModuleGenerator::generateUuid(const ShareableBytes& bytecode)

Can you rename to generateBytecodeHash?

::: js/src/wasm/WasmGenerator.cpp:1108
(Diff revision 3)
> +{
> +    mozilla::SHA1Sum::Hash hash;
> +    mozilla::SHA1Sum sha1Sum;
> +    sha1Sum.update(bytecode.begin(), bytecode.length());
> +    sha1Sum.finish(hash);
> +    MOZ_ASSERT(sizeof(ModuleHash) <= sizeof(mozilla::SHA1Sum::Hash));

can haz static_assert?

Attachment #8856759 - Flags: review?(luke) → review+

Comment hidden (mozreview-request)

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

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

Comment hidden (mozreview-request)

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

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

Comment hidden (mozreview-request)

Comment on attachment 8856759 [details]
Bug 1355263 - Generate better source URL for the wasm module.

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

Pulsebot

Comment 13

•

7 years ago

Pushed by ydelendik@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ed3b236c64c
Generate better source URL for the wasm module. r=luke

Iris Hsiao [:ihsiao]

Comment 14

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/4ed3b236c64c

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox55: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

Steve Fink [:sfink] [:s:]

Updated

•

7 years ago

Depends on: 1362807