In http://trac.webkit.org/changeset/76040 I removed some asserts that were getting triggered by calling window.open with an empty URL, but failed to add a test.
Created attachment 79315 [details] Patch
(Added the link to the bug to the ChangeLog)
Comment on attachment 79315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79315&action=review > LayoutTests/fast/dom/Window/open-window-empty-url.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Just <!DOCTYPE html> should be enough. That's what HTML5 recommends. > LayoutTests/fast/dom/Window/open-window-empty-url.html:21 > + <script type="text/javascript"> > + if (window.layoutTestController) { > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); > + layoutTestController.setCanOpenWindows(); > + } > + var newWindow = window.open(""); > + newWindow.close(); > + document.getElementById("results").innerText = "PASS: Calling window.open() with an empty URL did not trigger any assertions or cause any crashes."; > + if (window.layoutTestController) > + layoutTestController.notifyDone(); > + </script> > +</body> Since the whole test proceeds synchronously (before the page has even finished loading), there's no need for waitUntilDone/notifyDone.
(In reply to comment #3) > (From update of attachment 79315 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79315&action=review > > > LayoutTests/fast/dom/Window/open-window-empty-url.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > Just <!DOCTYPE html> should be enough. That's what HTML5 recommends. Done. > > > LayoutTests/fast/dom/Window/open-window-empty-url.html:21 > > + <script type="text/javascript"> > > + if (window.layoutTestController) { > > + layoutTestController.dumpAsText(); > > + layoutTestController.waitUntilDone(); > > + layoutTestController.setCanOpenWindows(); > > + } > > + var newWindow = window.open(""); > > + newWindow.close(); > > + document.getElementById("results").innerText = "PASS: Calling window.open() with an empty URL did not trigger any assertions or cause any crashes."; > > + if (window.layoutTestController) > > + layoutTestController.notifyDone(); > > + </script> > > +</body> > > Since the whole test proceeds synchronously (before the page has even finished loading), there's no need for waitUntilDone/notifyDone. Removed. Thanks for the review!
Comment on attachment 79315 [details] Patch Committed in r76084 http://trac.webkit.org/changeset/76084
This test crashes on the bots.
Build 23847 (r76085) was the first to show failures: set([u'fast/dom/Window/open-window-empty-url.html'])
Reverted r76084 for reason: Crashes on the bots Committed r76102: <http://trac.webkit.org/changeset/76102>
It's always sad to roll out a test case, but it seems the bug wasn't actually fixed before this test was added. :(
(In reply to comment #8) > Reverted r76084 for reason: > > Crashes on the bots > > Committed r76102: <http://trac.webkit.org/changeset/76102> Thanks for rolling it out. Which bots was it crashing on?
http://trac.webkit.org/changeset/76084 might have broken Chromium Linux Release
Leopard and Snow Leopard iirc. Sorry for not writing that in my rollout text.
<rdar://problem/8908391>
I changed the test case from the patch into JS Fiddle: Link - https://jsfiddle.net/sd65xt31/show It does not trigger any crash in Safari 15.6 on macOS 12.5 and it just triggers "Pop-up Blocker" and then after it shows (after allowing): ___ Test that calling window.open(""); does not trigger any assertions or cause any crashes. PASS: Calling window.open() with an empty URL did not trigger any assertions or cause any crashes. ___ It is same across all other browsers (Chrome Canary 106 and Firefox Nightly 104). I think if we don't need to land this test case, since it works across all browsers - we can close this as "RESOLVED CONFIGURATION CHANGED". Thanks!
This is about adding debug asserts. At this point, I don't think keeping this 11 year old bug is the way to address this. If we have had a need to add an assertion in this code, we would have done it meanwhile. -> Later.