Closed Bug 1503009 Opened 11 months ago Closed 11 months ago

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

Categories

(Core :: JavaScript Engine, defect)

63 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 + verified
firefox65 + verified

People

(Reporter: miketaylr, Assigned: jonco)

References

()

Details

(Keywords: regression)

Attachments

(4 files)

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
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
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.
I'll post a fix for central but we should back out bug 1484948 (and related bug 1492074) from beta.
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.
Attachment #9021277 - Flags: review?(jorendorff)
Comment on attachment 9021277 [details] [diff] [review]
bug1503009-import-support

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

:-|
Attachment #9021277 - Flags: review?(jorendorff) → review+
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
Attachment #9021488 - Flags: approval-mozilla-beta?
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
Attachment #9021489 - Flags: approval-mozilla-beta?
Pushed by jcoppeard@mozilla.com:
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 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.
Attachment #9021488 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9021489 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Should we add a test for this?
Flags: qe-verify+
Flags: needinfo?(jcoppeard)
Flags: in-testsuite?
I should have included this in the previous patch.

This test is for beta.  I'll land this on central minus the expected error.
Flags: needinfo?(jcoppeard)
Attachment #9021550 - Flags: review?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/97f81e96c9ab
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Attachment #9021550 - Flags: review?(jorendorff) → review+
Pushed by jcoppeard@mozilla.com:
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 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.
Attachment #9021550 - Flags: approval-mozilla-beta?
Comment on attachment 9021550 [details] [diff] [review]
bug1503009-beta-test

Test-only changes don't need approval to land :)
Attachment #9021550 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/releases/mozilla-beta/rev/cc49be946af9
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.