Closed Bug 1425076 Opened 2 years ago Closed Last year

Add a console warning message when running .wasm content with old unsupported debug symbols format

Categories

(Core :: JavaScript Engine, enhancement, P3)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: jujjyl, Assigned: luke)

Details

Attachments

(2 files, 1 obsolete file)

Currently due to product release timelines, the latest version of Unity still produces .wasm files with old debugging section format, and as result they do not get callstacks as Firefox (and Chrome and others) are unable to parse the old format.

This could cause frustration to users, as they have no way to understand what is going on here.

It would be good to add a warning message when compiling (or instantiating(?)) such a module that has the old names section to console log that says "This WebAssembly Module was produced with old incompatible names section format. Function names will not be available." or "Unable to parse WebAssembly names section. Function names will not be available." That would allow Unity to offer a troubleshooting/FAQ item on their page about how to deal with the issue.
That's a good idea.  We should just generally print a warning if any custom section fails to validate.
Priority: -- → P3
Finally got to this in the backlog.  I think this could be pretty useful from preventing a swamp of garbage name sections since people might actually notice and fix the errors.  In addition to unit tests, I also tested on a build with known-bad names (https://s3.amazonaws.com/mozilla-games/ZenGarden/EpicZenGarden.html) and known-good (https://kripken.github.io/embenchen/wasm/names.html).
Unrelated, but I've been meaning to do this for a while.
Assignee: nobody → luke
Attachment #8965141 - Flags: review?(bbouvier)
Attached patch warn-on-invalid-custom-section (obsolete) — Splinter Review
Pretty easy; mostly just piping a new Vector<UniqueChars> "warnings" array around everywhere we pipe the UniqueChars "error" message.

Note: there are apparently no uses of assertWarning(), which is why I was able to change the signature.
Attachment #8965143 - Flags: review?(bbouvier)
Updated so that unknown name subsections are skipped instead of warning.  Otherwise, emitting a perfectly-legal locals subsection:
  https://webassembly.github.io/spec/core/appendix/custom.html#subsections
which we don't currently decode would warn.
Attachment #8965520 - Flags: review?(bbouvier)
Attachment #8965143 - Attachment is obsolete: true
Attachment #8965143 - Flags: review?(bbouvier)
Comment on attachment 8965141 [details] [diff] [review]
rename-binary-iterator.h

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

Cool.

::: js/src/wasm/WasmOpIter.h
@@ +16,5 @@
>   * limitations under the License.
>   */
>  
> +#ifndef wasm_op_iter_h
> +#define wasm_op_iter_h

preexisting: please also fix #endif comment at the end of the file
Attachment #8965141 - Flags: review?(bbouvier) → review+
Comment on attachment 8965520 [details] [diff] [review]
warn-on-invalid-custom-section

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

Cool, thanks.

::: js/src/jit-test/tests/wasm/binary.js
@@ +453,5 @@
> +badNameSec3.body.pop();
> +assertWarning(() => wasmEval(moduleWithSections([v2vSigSection, declSec, bodySec, badNameSec3])), nameWarning);
> +assertNoWarning(() => wasmEval(moduleWithSections([nameSection([moduleNameSubsection('hi')])])));
> +assertWarning(() => wasmEval(moduleWithSections([nameSection([moduleNameSubsection('hi'), moduleNameSubsection('boo')])])), nameWarning);
> +assertNoWarning(() => wasmEval(moduleWithSections([nameSection([moduleNameSubsection('hi'), [4, 0]])])));

Can you add a comment above this test, like "unknown name subsection"?

::: js/src/wasm/WasmJS.cpp
@@ +863,5 @@
> +static bool
> +ReportCompileWarnings(JSContext* cx, const UniqueCharsVector& warnings)
> +{
> +    // Avoid spamming the console.
> +    size_t numWarnings = Min<size_t>(warnings.length(), 3);

If warnings.length() > 3, maybe worth adding an ultimate message saying "other warnings found, not displayed to avoid spamming the console"?

@@ +2330,5 @@
>  }
>  
>  static bool
> +Reject(JSContext* cx, const CompileArgs& args, Handle<PromiseObject*> promise,
> +       const UniqueChars& error)

nit: previous order was more respectful of our rule to put in-out parameters at the end of the signature?

(same happens for Resolve below, where a handle to importObj is passed before the warnings)

::: js/src/wasm/WasmValidate.cpp
@@ +242,5 @@
> +            warnf("in the '%s' custom section: %" PRIu32 " unconsumed bytes",
> +                  name, uint32_t(range.size - actualSize));
> +        else
> +            warnf("in the '%s' custom section: %" PRIu32 " bytes consumed past the end",
> +                  name, uint32_t(actualSize - range.size));

nit: please use {} for if/else.

@@ +244,5 @@
> +        else
> +            warnf("in the '%s' custom section: %" PRIu32 " bytes consumed past the end",
> +                  name, uint32_t(actualSize - range.size));
> +        finishSkippedCustomSection(range);
> +        return;

nit: useless return

@@ +251,5 @@
> +    // Nothing to do! (c.f. finishSkippedCustomSection())
> +}
> +
> +void
> +Decoder::finishSkippedCustomSection(const SectionRange& range)

nit: the name confused me and suggested that the section had been skipped already. How about skipAndFinishCustomSection instead?

Thinking about it just a bit more: "skip" would even be enough. Maybe we could rename the current `skipCustomSection` to `skipAnyCustomSection` and name this one `skipCustomSection` instead?
Attachment #8965520 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
Thanks, good catches!

> > +Reject(JSContext* cx, const CompileArgs& args, Handle<PromiseObject*> promise,
> > +       const UniqueChars& error)
> 
> nit: previous order was more respectful of our rule to put in-out parameters
> at the end of the signature?
> 
> (same happens for Resolve below, where a handle to importObj is passed
> before the warnings)

I don't quite follow in both cases: 'promise' and 'importObj' are in-params (a MutableHandle would be in-out) nor are the referenced objects mutated in an indirect outparam-y way.

> @@ +244,5 @@
> > +        else
> > +            warnf("in the '%s' custom section: %" PRIu32 " bytes consumed past the end",
> > +                  name, uint32_t(actualSize - range.size));
> > +        finishSkippedCustomSection(range);
> > +        return;
> 
> nit: useless return

Technically, but that depends on the coincidental property that there is nothing left to do in the valid-custom-section case.  Should something be added later, it would be easy to not notice the missing return.  Another reason is symmetry with the preceding finishSkippedCustomSection() case.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f589a76063
Baldr: rename WasmBinaryIterator to OpIter (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/350013b0e02d
Baldr: warn on invalid name section (r=bbouvier)
https://hg.mozilla.org/mozilla-central/rev/d9f589a76063
https://hg.mozilla.org/mozilla-central/rev/350013b0e02d
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.