Bug 206445

Summary: window proxy of detached iframe doesn't respect updates to global values
Product: WebKit Reporter: John-David Dalton <john.david.dalton>
Component: WebCore JavaScriptAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, fpizlo, ggaren, keith_miller, leonardo.balter, mark.lam, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test
none
Patch
none
Patch for landing ews-feeder: commit-queue-

John-David Dalton
Reported 2020-01-17 14:55:47 PST
<html> <head> </head> <body> <script> const iframe = document.createElement('iframe'); document.body.appendChild(iframe); const { contentWindow: { eval: iframeEval } } = iframe; iframeEval(` log = top.console.log; foo = 1 `); iframe.remove(); iframeEval(` foo++; if (foo !== 2) { log('fails!'); } else { log('works!'); } `); </script> </body> </html> Logs "fails!" in Safari but works in Chrome and Firefox: See https://output.jsbin.com/yuzeyup/1/
Attachments
Test (2.54 KB, patch)
2021-03-02 17:59 PST, Keith Miller 		no flags
DetailsFormatted DiffDiff
Patch (11.96 KB, patch)
2021-03-03 23:56 PST, Keith Miller 		no flags
DetailsFormatted DiffDiff
Patch for landing (12.32 KB, patch)
2021-03-04 09:44 PST, Keith Miller 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-01-17 22:56:12 PST
<rdar://problem/58706578>
Chris Dumez
Comment 2 2020-05-29 14:47:49 PDT
foo is 1 before and after foo++ Somehow, we are unable to modify foo variable, likely because the iframe where the script is running was removed from the document.
Chris Dumez
Comment 3 2020-05-29 14:51:06 PDT
A simpler way to write this: <html> <body> <script> const iframe = document.createElement('iframe'); document.body.appendChild(iframe); const iframeEval = iframe.contentWindow.eval; iframeEval(` log = top.console.log; foo = 1 `); iframe.remove(); iframeEval(` foo++; if (foo !== 2) { log('fails!'); } else { log('works!'); } `); </script> </body> </html>
Leo Balter
Comment 4 2021-02-17 12:08:10 PST
This issue persists today and it's being challenging for proper workarounds. Fixing this would be highly appreciated and I wonder how I could help. I tried to reduce the example to have it represented in Test262 but it is directly connected to the detached iframe.
Keith Miller
Comment 5 2021-03-02 17:58:03 PST
Looks like we have block all stores to windows when they no longer have a frame, which doesn't appear to be correct from the spec nor does it match FF/Chrome. I think we should only block if the window without a frame is also not the lexical global object.
Keith Miller
Comment 6 2021-03-02 17:59:37 PST
Created attachment 422029 [details] Test
Geoffrey Garen
Comment 7 2021-03-02 20:38:01 PST
Comment on attachment 422029 [details] Test View in context: https://bugs.webkit.org/attachment.cgi?id=422029&action=review > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:330 > + if (thisObject != lexicalGlobalObject && !thisObject->wrapped().frame()) I think this would be clearer as an explicit early return of true when thisObject == lexicalGlobalObject. But also, I'm not sure this is the best fix. Perhaps instead we should remove the frame() checks entirely. Here's a test case: <html> <body> <script> const iframe = document.createElement('iframe'); document.body.appendChild(iframe); const iframeContentWindow = iframe.contentWindow; iframeContentWindow.foo = 1; iframe.remove(); iframeContentWindow.foo++; if (iframeContentWindow.foo !== 2) { console.log('fails!'); console.log(iframeContentWindow.foo); } else { console.log('works!'); console.log(iframeContentWindow.foo); } </script> </body> </html> This "works" in Firefox (and "fails" in Chrome). I wonder what the spec says about this case.
Leo Balter
Comment 8 2021-03-02 21:01:09 PST
Thank you, Keith and Geoffrey! Geoffrey, your example is working both in Chrome (88.0.4324.192 x86_64) and Firefox (86.0) for me, still failing in Safari. It fails in Chrome only if I open the html file locally, but it works using http (locally or via jsfiddle). For reference: https://jsfiddle.net/d457haLp/
Chris Dumez
Comment 9 2021-03-03 11:26:12 PST
Comment on attachment 422029 [details] Test View in context: https://bugs.webkit.org/attachment.cgi?id=422029&action=review >> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:330 >> + if (thisObject != lexicalGlobalObject && !thisObject->wrapped().frame()) > > I think this would be clearer as an explicit early return of true when thisObject == lexicalGlobalObject. > > But also, I'm not sure this is the best fix. Perhaps instead we should remove the frame() checks entirely. Here's a test case: > > <html> > <body> > <script> > const iframe = document.createElement('iframe'); > document.body.appendChild(iframe); > const iframeContentWindow = iframe.contentWindow; > iframeContentWindow.foo = 1; > > iframe.remove(); > > iframeContentWindow.foo++; > > if (iframeContentWindow.foo !== 2) { > console.log('fails!'); > console.log(iframeContentWindow.foo); > } else { > console.log('works!'); > console.log(iframeContentWindow.foo); > } > </script> > </body> > </html> > > This "works" in Firefox (and "fails" in Chrome). I wonder what the spec says about this case. I suggest we add such a test for a cross-origin iframe too because we would not want to introduce any security bugs :)
Geoffrey Garen
Comment 10 2021-03-03 12:39:31 PST
(In reply to Leo Balter from comment #8) > Thank you, Keith and Geoffrey! > > Geoffrey, your example is working both in Chrome (88.0.4324.192 x86_64) and > Firefox (86.0) for me, still failing in Safari. > > It fails in Chrome only if I open the html file locally, but it works using > http (locally or via jsfiddle). > > For reference: https://jsfiddle.net/d457haLp/ Thanks for testing! Weird that Chrome's behavior differs between local file an http. In either case, though, this seems to confirm that remove the frame() check is probably the best fix. (And yeah, Chris is right that we should add a cross-origin test that is denied, too.)
Keith Miller
Comment 11 2021-03-03 23:56:23 PST
Created attachment 422185 [details] Patch
Chris Dumez
Comment 12 2021-03-04 08:32:39 PST
Comment on attachment 422185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422185&action=review r=me with nits. > LayoutTests/fast/frames/iframe-detached-window-still-writable-eval.html:3 > +<script src="../../resources/js-test-pre.js"></script> We just include ./../resources/js-test.js nowadays. > LayoutTests/fast/frames/iframe-detached-window-still-writable-eval.html:5 > + const iframe = document.createElement('iframe'); Please add a description("explanation of that this test does"); > LayoutTests/fast/frames/iframe-detached-window-still-writable-eval.html:22 > +<script src="../../resources/js-test-post.js"></script> Not needed with my suggestion above. > LayoutTests/fast/frames/iframe-detached-window-still-writable.html:3 > +<script src="../../resources/js-test-pre.js"></script> no -pre > LayoutTests/fast/frames/iframe-detached-window-still-writable.html:5 > + const iframe = document.createElement('iframe'); Please add a description(); > LayoutTests/fast/frames/iframe-detached-window-still-writable.html:13 > +<script src="../../resources/js-test-post.js"></script> Not needed.
Keith Miller
Comment 13 2021-03-04 09:41:57 PST
Comment on attachment 422185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422185&action=review >> LayoutTests/fast/frames/iframe-detached-window-still-writable-eval.html:3 >> +<script src="../../resources/js-test-pre.js"></script> > > We just include ./../resources/js-test.js nowadays. Oh cool! Good to know. >> LayoutTests/fast/frames/iframe-detached-window-still-writable-eval.html:5 >> + const iframe = document.createElement('iframe'); > > Please add a description("explanation of that this test does"); done and ditto below.
Keith Miller
Comment 14 2021-03-04 09:44:57 PST
Created attachment 422243 [details] Patch for landing
EWS
Comment 15 2021-03-04 10:26:27 PST
Committed r273901: <https://commits.webkit.org/r273901> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422243 [details].
Note You need to log in before you can comment on or make changes to this bug.