Bug 52654

Summary: Add a test that calling window.open with an empty URL does not trigger an assertion or cause a crash
Product: WebKit Reporter: Jessie Berlin <jberlin>
Component: WebKit2Assignee: Jessie Berlin <jberlin>
Status: RESOLVED LATER    
Severity: Normal CC: abarth, ahmad.saleem792, ap, aroben, bfulgham, eric, jberlin, rniwa, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Jessie Berlin
Reported 2011-01-18 12:56:48 PST
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.
Attachments
Patch (2.28 KB, patch)
2011-01-18 12:59 PST, Jessie Berlin 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jessie Berlin
Comment 1 2011-01-18 12:59:29 PST
Created attachment 79315 [details] Patch
Jessie Berlin
Comment 2 2011-01-18 13:01:03 PST
(Added the link to the bug to the ChangeLog)
Adam Roben (:aroben)
Comment 3 2011-01-18 13:08:32 PST
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.
Jessie Berlin
Comment 4 2011-01-18 17:07:50 PST
(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!
Jessie Berlin
Comment 5 2011-01-18 17:14:25 PST
Comment on attachment 79315 [details] Patch Committed in r76084 http://trac.webkit.org/changeset/76084
Eric Seidel (no email)
Comment 6 2011-01-18 22:04:48 PST
This test crashes on the bots.
Eric Seidel (no email)
Comment 7 2011-01-18 22:05:11 PST
Build 23847 (r76085) was the first to show failures: set([u'fast/dom/Window/open-window-empty-url.html'])
Eric Seidel (no email)
Comment 8 2011-01-18 22:09:41 PST
Reverted r76084 for reason: Crashes on the bots Committed r76102: <http://trac.webkit.org/changeset/76102>
Eric Seidel (no email)
Comment 9 2011-01-18 22:10:23 PST
It's always sad to roll out a test case, but it seems the bug wasn't actually fixed before this test was added. :(
Adam Roben (:aroben)
Comment 10 2011-01-19 05:09:09 PST
(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?
WebKit Review Bot
Comment 11 2011-01-19 06:09:00 PST
http://trac.webkit.org/changeset/76084 might have broken Chromium Linux Release
Eric Seidel (no email)
Comment 12 2011-01-19 10:54:20 PST
Leopard and Snow Leopard iirc. Sorry for not writing that in my rollout text.
Jessie Berlin
Comment 13 2011-01-24 12:16:44 PST
<rdar://problem/8908391>
Ahmad Saleem
Comment 14 2022-07-26 09:44:13 PDT
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!
Ryosuke Niwa
Comment 15 2022-07-26 10:05:36 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.