1395896 - EXCEPTION_ACCESS_VIOLATION_READ using JS module

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.101 Safari/537.36

Steps to reproduce:

1. enable dom.moduleScripts.enabled pref 
2. Go to https://test.shhnjk.com/test.html
3 Reload the page


Actual results:

Crash.
https://crash-stats.mozilla.com/report/index/f974d23e-b867-4b19-9741-3db9e0170901


Expected results:

should not crash.

Tested on latest Nightly on Windows 10.

Comment 1

[:Gijs (he/him)]

:jonco, seems the pref mentioned in comment #0 was added in a patch you wrote, so I'm hoping you know who can look at this? :-)

Not entirely sure this needs to be sec-sensitive, crash looks like a near-null read to me (but please check, not an expert).

Comment 2

[Jon Coppeard (:jonco)]

This is not sec-sensitive as JS modules are disabled by default.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1395896-mixed-content-import

The problem is that ModuleLoadRequest::ModuleErrored() asserts that the module script is null or errored after the status of all a module's dependencies are checked, but in ScriptLoader::StartFetchingModuleAndDependencies() we can hit an error loading a dependency before we we add that dependency to the list in the load request and hence it appears that the module's dependencies all loaded OK. 

The fix is just to add dependent load requests to the list before we try to load them.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8904246 [details] [diff] [review]
bug1395896-mixed-content-import

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

Thanks for adding a test!

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Daniel, does this really need to be private given the feature is disabled by default?

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0be76a0e23a2b3df6be30b4c0cce6b0b94fb367d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0be76a0e23a2

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> Daniel, does this really need to be private given the feature is disabled by default?

Since it's fixed, no. In general if not fixed it would depend on how confident you are we'd fix a given security bug before enabling the feature.