Crash when importing a data URL in the Live DOM Viewer
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: annevk, Assigned: jonco)
References
Details
(4 keywords, Whiteboard: [post-critsmash-triage][adv-main66+][adv-esr60.6+])
Crash Data
Attachments
(6 files)
3.21 KB,
patch
|
bzbarsky
:
review-
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
2.46 KB,
patch
|
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
47 bytes,
text/x-phabricator-request
|
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
5.36 KB,
patch
|
jonco
:
review+
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
- Go to https://software.hixie.ch/utilities/js/live-dom-viewer/
- Replace the text in the top text field with
<script> x = import("data:text/javascript,") </script>
- Add a space or some such.
- ...
- Crash.
I cannot reproduce it outside the Live DOM Viewer unfortunately.
Comment 1•6 years ago
|
||
Adding the crash signature as this is easily reproducible on Mac.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
I can't reproduce this myself, but here's the stack:
MOZ_CrashPrintf
js::ContextChecks::check(JS::Value const&, int)
JS_SetPendingException(JSContext*, JS::Handle<JS::Value>)
mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*)
mozilla::dom::ScriptRequestProcessor::Run()
mozilla::dom::ScriptLoader::ProcessLoadedModuleTree(mozilla::dom::ModuleLoadRequest*)
mozilla::dom::ModuleLoadRequest::ModuleErrored()
mozilla::MozPromise<bool, nsresult, false>::ThenValue<mozilla::dom::ModuleLoadRequest*, void (mozilla::dom::ModuleLoadRequest::)(), void (mozilla::dom::ModuleLoadRequest::)()>::DoResolveOrRejectInternal(mozilla::MozPromise<bool, nsresult, false>::ResolveOrRejectValue&)
From: https://crash-stats.mozilla.com/report/index/71bd0b74-8686-45ac-be6a-a19d00190220
Assignee | ||
Comment 3•6 years ago
|
||
Setting s-s for compartment mismatch assertion.
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
I still can't reproduce this or see how this can happen.
Marcia, does it still reproduce for you? STR would be invaluable.
Assignee | ||
Comment 5•6 years ago
|
||
It's possible that this might be related to bug 1530146.
Comment 6•6 years ago
|
||
I was able to reproduce this I guess in the nightly from 2-20, but not yet using today's nightly. As far as STR, I followed the steps exactly in Comment 0.
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Marcia Knous [:marcia - needinfo? me] from comment #6)
Thanks. I get an immediate crash using a build from that date.
Assignee | ||
Comment 8•6 years ago
|
||
I bisected this and found it was fixed by bug 1523843.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 9•6 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #8)
I bisected this and found it was fixed by bug 1523843.
Hm that bug made us have fewer compartments but are we sure the underlying bug is fixed too? There are still compartment boundaries elsewhere..
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #9)
OK that's a really good point.
bz, is it possible for a Document object to be used with more than one global in its lifetime?
Comment 11•6 years ago
•
|
||
bz, is it possible for a Document object to be used with more than one global in its lifetime?
Yes, with document.open, which is exactly what's going on here. A minimized testcase for this bug:
<!DOCTYPE HTML>
<iframe></iframe>
<script>
function doIt() {
var doc = document.querySelector("iframe").contentDocument;
doc.open();
doc.write('<script>import("data:text/javascript,")</' + 'script>');
doc.close();
}
</script>
<button type="button" onclick="doIt()">
Click this button, then wait a second and click it again.
</button>
Even with bug 1523843 fixed, this is an issue for the moment, as this testcase shows:
<!DOCTYPE HTML>
<script>
var win;
function doIt() {
if (!win) {
win = window.open();
}
var doc = win.document;
doc.open();
doc.write('<script>import("data:text/javascript,")</' + 'script>');
doc.close();
}
</script>
<button type="button" onclick="doIt()">
Click this button, then wait a second and click it again.
</button>
Bug 1489308 will fix the issue by removing the "single document with multiple globals" situation. But that's not landed yet, so we need a different fix for Firefox 66, right?
Updated•6 years ago
|
Updated•6 years ago
|
Comment 12•6 years ago
|
||
Maybe document.open should clear out the module map bits in the scriptloader at the point where it changes the global for the document...
Comment 13•6 years ago
|
||
Is 66 affected?
Comment 14•6 years ago
|
||
That is what the needinfo is for, to a large extent.
Is this only a problem for dynamic imports, or also for static ones? Dynamic imports seem to be enabled in nightly only at the moment. Static imports have been shipping for a while.
The testcases above are using dynamic imports and don't crash on 66b11: they throw an exception about dynamic import not being supported, as expected.
I tried creating something like that with static imports, but it does not crash in my testing so far. That said, maybe I'm just managing to exercise the right codepaths...
If this is a nightly-only issue, then we're done here; I just landed bug 1489308 earlier today. Maybe it's worth adding a check of some sort to ensure that we don't run modules in the wrong compartment, though...
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #11)
Thanks for the testcases!
The bad news is that this problem does apply to static imports and hence everything back to FF 60 is affected.
Updated•6 years ago
|
Assignee | ||
Comment 16•6 years ago
|
||
Patch to fix the problem by clearing the module map when we change global for a Document.
Assignee | ||
Comment 17•6 years ago
|
||
Patch to change the compartment checks around modules (including JS_SetPendingException which is called when a module load failure is reported at a later time) into release build checks.
Jan what do you think? Most of our compartment checks are nightly-only but I don't think this is going to be too much overhead.
Assignee | ||
Comment 18•6 years ago
|
||
This is where I got to with tests. The 1529203-1.html test functions correctly but the other two which use window.open() still don't work, with window.open() returning null even with the pref set.
Any more ideas on how to make this work?
Updated•6 years ago
|
Comment 19•6 years ago
|
||
Comment 20•6 years ago
|
||
Comment 21•6 years ago
|
||
It's also possible that setting pref(browser.link.open_newwindow,2) for the tests (to open in new windows, not new tabs) will work in crashtests...
Comment 22•6 years ago
|
||
Assignee | ||
Comment 23•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
Depends on D21718
Assignee | ||
Comment 25•6 years ago
|
||
Comment on attachment 9047746 [details]
Bug 1529203 - Clear the module map when changing a Document's global r=bzbarsky!
Security Approval Request
- How easily could an exploit be constructed based on the patch?: I'm not sure how to estimate this. Possible but not easy I guess.
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
- Which older supported branches are affected by this flaw?: Everything back to FF 60
- If not all supported branches, which bug introduced the flaw?: Bug 1240072
- Do you have backports for the affected branches?: No
- If not, how different, hard to create, and risky will they be?: Should be simple to create and low risk.
- How likely is this patch to cause regressions; how much testing does it need?: Unlikely, this is a quite a simple change.
Assignee | ||
Comment 26•6 years ago
|
||
Comment on attachment 9047746 [details]
Bug 1529203 - Clear the module map when changing a Document's global r=bzbarsky!
Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: Bug 1240072
- User impact if declined: Possible crash / security vulnerability.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- 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 simple change to clear the module map when a document's global is changed via window.open. I'd say it's low risk.
- String changes made/needed:
Assignee | ||
Comment 27•6 years ago
|
||
I'm planning to submit all patches to beta, but only the tests and extra assertions to trunk as this issue is no longer present there due to the changes in bug 1489308.
Comment 28•6 years ago
|
||
Note, it's probably best not to check in the tests until the bug is unhidden.
Comment 29•6 years ago
|
||
Comment on attachment 9047746 [details]
Bug 1529203 - Clear the module map when changing a Document's global r=bzbarsky!
Sec-approval+ for trunk and beta approval.
Since you state this goes back to Firefox 60, we should get an ESR60 patch made and nominated as well.
Assignee | ||
Comment 30•6 years ago
|
||
Assignee | ||
Comment 31•6 years ago
|
||
Updated•6 years ago
|
Comment 32•6 years ago
|
||
Assignee | ||
Comment 33•6 years ago
|
||
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fix for sec-high bug.
- User impact if declined: Possible crash / security vulnerability.
- Fix Landed on Version: 66
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This is a simple change and shouldn't cause problems.
- String or UUID changes made by this patch: None
Comment 34•6 years ago
|
||
Comment 35•6 years ago
|
||
uplift |
Updated•6 years ago
|
Comment 36•6 years ago
|
||
I managed to reproduce the issue on an older version of Nightly (2019-02-20) on Windows 10 x64.
I retested everything using latest Nightly 67.0a1, beta 66.0b13 and esr 60.5.3 and the tab doesn't crash anymore.
However, I tried the steps on Firefox 65.0.2 and the bug is not reproducing. Considering the fact that the flag at the moment is wontfix, did something happened or that build wasn't affected in the first place?
Assignee | ||
Comment 37•6 years ago
|
||
(In reply to Oana Botisan, Desktop Release QA from comment #36)
These STR will only work on FF 66 and later, but the problem itself exists from FF 60.
Comment 38•6 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #37)
These STR will only work on FF 66 and later, but the problem itself exists from FF 60.
Thank you for the info. Considering the fact that firefox65 was flagged with wontfix, I assume it's not an issue.
Then I will mark this bug as verified fixed as per comment 36.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Comment 39•4 years ago
|
||
Comment 40•4 years ago
|
||
Backed out changeset 9133eeb54db8 (bug 1529203) for Crashtests failures in dom/base/crashtests/1529203-2.html. CLOSED TREE
Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=303370464&repo=autoland&lineNumber=2849
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=API845cEQyuaf3aNDkBKZA-0&revision=9133eeb54db8be5f55e51576ae07a1113d48f1f5
Backout:
https://hg.mozilla.org/integration/autoland/rev/0c7f55b96d599ccdf605abd903ed3b6a62187ae3
Updated•4 years ago
|
Comment 41•4 years ago
|
||
Comment 42•4 years ago
|
||
bugherder |
Assignee | ||
Updated•4 years ago
|
Description
•