<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/
<rdar://problem/58706578>
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.
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>
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.
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.
Created attachment 422029 [details] Test
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.
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/
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 :)
(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.)
Created attachment 422185 [details] Patch
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.
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.
Created attachment 422243 [details] Patch for landing
Committed r273901: <https://commits.webkit.org/r273901> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422243 [details].