Closed Bug 1342060 Opened 3 years ago Closed 3 years ago

wasm: enable by default

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- disabled
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 1 obsolete file)

Chrome has already enabled wasm by default on all channels, the binary format and API are stable, and there's no blockers I'm aware of for enabling starting with 52.
Attached patch enable-by-default (obsolete) — Splinter Review
This patch both enables wasm and stops accepting the experimental 0xd version (only 0x1 is accepted now).  Because of this, a number of tests had to be updated that baked in 0xd.
Attachment #8840486 - Flags: review?(bbouvier)
Comment on attachment 8840486 [details] [diff] [review]
enable-by-default

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

The technical parts look great to me.
Attachment #8840486 - Flags: review?(bbouvier) → review+
Comment on attachment 8840486 [details] [diff] [review]
enable-by-default

Requesting sr for enabling this feature in SM.
Attachment #8840486 - Flags: superreview?(jorendorff)
Oops, try server found some extra tests that needed to be modified.
Attachment #8840486 - Attachment is obsolete: true
Attachment #8840486 - Flags: superreview?(jorendorff)
Attachment #8840510 - Flags: superreview?(jorendorff)
Attachment #8840510 - Flags: review+
Attachment #8840510 - Flags: superreview?(jorendorff) → superreview+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/391047948db4
wasm: enable by default (r=bbouvier,sr=jorendorff)
Attached patch aurora patchSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: enabling wasm
[User impact if declined]: no wasm
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not with the patch itself; shipping a new feature is inherently risky, of course, but this is what we've been addressing with several forms of fuzzing, large compiled codebases, shared test suites, all of which has applied to what's in aurora/beta)
Attachment #8840543 - Flags: review+
Attachment #8840543 - Flags: approval-mozilla-aurora?
Approval Request Comment
[Feature/Bug causing the regression]: enabling wasm on beta
[User impact if declined]: no wasm
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: see comment 7
[Why is the change risky/not risky?]: see comment 7
Attachment #8840545 - Flags: review+
Attachment #8840545 - Flags: approval-mozilla-beta?
Comment on attachment 8840545 [details] [diff] [review]
enable-by-default

Enable all the wasms!
Attachment #8840545 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8840543 [details] [diff] [review]
aurora patch

Aurora wasm incoming!
Attachment #8840543 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Oops, I missed in the try results that on some of the old android versions we test on, wasm compilation is disabled and so the global WebAssembly object is not exposed on those platforms.  This patch fixes the window/worker/service-worker cases.  In a window, we can use SpecialPowers to ensure that the reason for a missing WebAssembly object is because it's not supported by the platform.  But in workers, there are no SpecialPowers so the best I could think of was to just mark it as "optional".
Attachment #8840636 - Flags: review?(jorendorff)
Comment on attachment 8840636 [details] [diff] [review]
fix-android-tests

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

OK.
Attachment #8840636 - Flags: review?(jorendorff) → review+
(In reply to Douglas Crosher [:dougc] from comment #13)
Your dispute is noted.  I understand that there are many things that you would like to have changed in wasm.  At this point, though, browsers have converged on a common, stable core that can serve as a first shippable unit and that is already enabled by default in Chrome.  Going forward, there's a lot of room for incremental improvement and it will be much easier to make these complex value judgements once we see a lot more kinds of usage of wasm which we can use to evaluate future options.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0187aa7969c7
Followup to 391047948db4 to fix Android tests (r=jorendorff)
(The test code fixed by the follow-up patch moved between m-i and m-a.)
https://hg.mozilla.org/mozilla-central/rev/391047948db4
https://hg.mozilla.org/mozilla-central/rev/0187aa7969c7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
This was re-disabled for ESR52 in bug 1342440.
Depends on: 1430224
You need to log in before you can comment on or make changes to this bug.