1503009 - doctorn.rocks - missing content due to dynamic module import syntax error

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Taylor [:miketaylr]]

Originally filed @ https://github.com/webcompat/web-bugs/issues/20396

12:04.81 INFO: Last good revision: 293c4672dd15110112e96e277a30e3180133278d
12:04.81 INFO: First bad revision: d1b2141b1c454f28b8d35164c958e9ddcc7058fe
12:04.81 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=293c4672dd15110112e96e277a30e3180133278d&tochange=d1b2141b1c454f28b8d35164c958e9ddcc7058fe

STR:

1. load https://doctorn.rocks/travel-hub/

expected: table with data
actual: no data, error message in console:

> SyntaxError: dynamic module import is not implemented client.46e573ad.js:488:4

Comment 1

[Jon Coppeard (:jonco)]

The site is using this code detect whether dynamic module import is implemented:

  try { new Function("if(0)import('')")(); ... } 

Surprisingly, this is not throwing despite the fact that import() is not supported in the browser.

I'm pretty sure this is because the check to see whether it's supported is happening in the bytecode emitter, and the folding pass is removing the body of the if statement, because it's never executed.  

Because this doesn't throw, the site goes on to try to use import() for real and it fails.

Comment 2

[Jon Coppeard (:jonco)]

I'll post a fix for central but we should back out bug 1484948 (and related bug 1492074) from beta.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1503009-import-support

The problem is as described above.  Since we now support import() in the shell in 65 we can move the check to the parser where it really belongs.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 9021277 [details] [diff] [review]
bug1503009-import-support

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

:-|

Comment 5

[Jon Coppeard (:jonco)]

Attachment: backout-805f1a2737a0

Requesting approval to back out bug 1492074 from 64 to fix this issue.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1492074

User impact if declined: Web compatibility issue

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a backout of bug 1492074 so should be low risk.

String changes made/needed: None

Comment 6

[Jon Coppeard (:jonco)]

Attachment: backout-d1b2141b1c45

Requesting approval to back out bug 1484948 from 64 to fix this issue.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1484948

User impact if declined: Web compatibility issue

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a backout of bug 1484948 so should be low risk.

String changes made/needed: None

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97f81e96c9ab
If dynamic module import is not supported its use should be rejected at parse time r=jorendorff

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9021488 [details] [diff] [review]
backout-805f1a2737a0

[Triage Comment]
Fixes a new regression in Fx64 by backing out the regressing patches. Approved for 64.0b6.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd2b79631e52
https://hg.mozilla.org/releases/mozilla-beta/rev/c2d642cb5eb2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Should we add a test for this?

Comment 11

[Jon Coppeard (:jonco)]

Attachment: bug1503009-beta-test

I should have included this in the previous patch.

This test is for beta.  I'll land this on central minus the expected error.

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/97f81e96c9ab

Comment 13

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce this issue using Firefox 65.0a1 (BuildId:20181029230149) on Windows 10 64bit.

This issue is verified fixed using Firefox 65.0a1 (BuildId:20181031223503) and the provided Firefox 64.0b6 build in comment 9 on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 32bit.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/893565d632d8
Test that import() is a syntax error even if we don't emit bytecode for it r=jorendorff

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 9021550 [details] [diff] [review]
bug1503009-beta-test

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1484948

User impact if declined: Requesting approval to land test case on beta.  No user impact.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It's test code only.

String changes made/needed: None.

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/893565d632d8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9021550 [details] [diff] [review]
bug1503009-beta-test

Test-only changes don't need approval to land :)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc49be946af9