Multiple correctness issues with javascript URLs 
https://bugs.webkit.org/show_bug.cgi?id=16855
Summary Multiple correctness issues with javascript URLs
Adam Barth
Reported 2008-01-12 11:02:30 PST
WebKit's implementation of javascript URLs has a number of bugs: 1) Targeted hyperlinks and forms to javascript URLs do not run in the targeted window. (Instead, the run in the window with the hyperlink or form.) Note, be careful to check that the active frame is allowed to script the target frame before executing these javascript URLs. 2) javascript URL only replace the current document if they return a primitive string. They should replace the document if they return a non-undefined value. 3) As of r29432, some methods of invoking javascript URLs do not replaced the document when they return a value. 4) One implementation of FrameLoader::urlSelected executes javascript: URLs, but the other does not. I don't know how to poke this, but it seems like a bug. I'll attach LayoutTests shortly (They are also hosted at <http://crypto.stanford.edu/~abarth/research/webkit/jstests/>). Compare their behavior in WebKit to their behavior in Firefox and IE7.
Attachments
LayoutTests for these issues (21.71 KB, patch)
2008-01-12 11:04 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
12161.1.txt (4.36 KB, patch)
2009-06-04 17:20 PDT, Michael Nordman 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2008-01-12 11:04:16 PST
Created attachment 18408 [details] LayoutTests for these issues Attached are some layout tests for these issues. These layout tests do not test for security, just for correctness. (Also, they might need to be tweaked to run correctly in DRT.)
David Kilzer (:ddkilzer)
Comment 2 2008-01-12 11:25:18 PST
<rdar://problem/5685381>
Alexey Proskuryakov
Comment 3 2008-01-12 14:27:53 PST
See also: bug 9706.
Geoffrey Garen
Comment 4 2008-02-14 15:49:54 PST
See also bug 17367 and bug 17368.
Michael Nordman
Comment 5 2009-06-04 17:15:55 PDT
Also see http://code.google.com/p/chromium/issues/detail?id=12161 Chrome has a P1 crashing bug around this. In cases where the script url constructs a document that initiates subresource loads as follows... <head> <script language="JavaScript"> function FrameContents() { var doc = theFrame.document; doc.open(); doc.write('<img src=image.png>'); doc.close(); return ""; } </script> </head> <frameset> <frame name="theFrame" target=menu src="javascript:parent.FrameContents()"> </frameset> I have a patch addresses both the crash and the correctness issue... the constructed document should take precedence of the script's return value in this case (IE and FF do that).
Michael Nordman
Comment 6 2009-06-04 17:20:42 PDT
Created attachment 30975 [details] 12161.1.txt Here's the patch mentioned in the previous comment. The crash I'm seeing is because the Document (and relatives) is being deleted, but resource loads are still pending. The 'fix' for that in here feels hacky. Seems like stopAllLoaders() should have taken care of things? The fix for he correctness issue feels more correct to me. All existing layout test pass with this patched in. I'm looking into making additional layout test for javascript urls of this form.
Adam Barth
Comment 7 2009-06-05 00:43:26 PDT
This really feels like it should be two patches: 1) A patch to fix the correctness issue (with a LayoutTest showing the different behavior). 2) A patch to fix the crasher (also with a LayoutTest showing the crasher is gone).
Michael Nordman
Comment 8 2009-06-05 08:51:06 PDT
(In reply to comment #7) > This really feels like it should be two patches: Ok. > crasher fix I'm not real happy with this change. Any loader gurus out there see a more appropriate answer? A symptom of the problem is that after calling stopAllLoaders(), the existing docs DocLoader->m_requestCount field has not gone to zero. The code that follows the stop call causes the existing doc to get blown away. The assertion in DocLoader's dtor fires at this point (ASSERT(m_requestCount == 0). At some later point, when those still active requests make progress of some kind, chrome crashes. Should stopAllLoaders() also be responsible for terminating these requests too or not? If the intent was that it should terminate these requests too... i can make a change in there to ensure that it does so. Its just not clear to me if that is the intent of this method? The crash is specific to chrome (perhaps browsers employing v8 more specifically). With safari (jsc), the sequence of events is different. I haven't drilled into where things are different just yet. I do know that if the pending requests are killed... crash fixed.
Michael Nordman
Comment 9 2009-06-05 19:03:19 PDT
See bug 26230 I split that off for the chrome specific crasher instead of continuing to hijack this general issue for that specific one. Also the stab at support for respecting programatically generate documents (in patch 12161.1.txt) seems like a good start... but its not sufficient... layout/rendering ceases immediate after the first frame to employ that technique. So we still have botched pages. I'm dropping that to focus on the blocking crasher.
Note You need to log in before you can comment on or make changes to this bug.