Bug 16855

Summary:

Multiple correctness issues with javascript URLs

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

Frames

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Normal

CC:

abarth, ahmad.saleem792, annevk, ap, collinj, emacemac7, ian.eng.webkit, mal, michaeln, mitz, sam

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://crypto.stanford.edu/~abarth/research/webkit/jstests/

Attachments:

Description	Flags
LayoutTests for these issues	none
12161.1.txt	none

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	Details Formatted Diff Diff
12161.1.txt (4.36 KB, patch) 2009-06-04 17:20 PDT, Michael Nordman	no flags	Details Formatted Diff Diff
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 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.