Bug 16855 - Multiple correctness issues with javascript URLs
Summary: Multiple correctness issues with javascript URLs
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://crypto.stanford.edu/~abarth/re...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-01-12 11:02 PST by Adam Barth
Modified: 2010-06-10 17:33 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 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.
Comment 1 Adam Barth 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.)
Comment 2 David Kilzer (:ddkilzer) 2008-01-12 11:25:18 PST
<rdar://problem/5685381>
Comment 3 Alexey Proskuryakov 2008-01-12 14:27:53 PST
See also: bug 9706.
Comment 4 Geoffrey Garen 2008-02-14 15:49:54 PST
See also bug 17367 and bug 17368.
Comment 5 Michael Nordman 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). 
Comment 6 Michael Nordman 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.
Comment 7 Adam Barth 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).
Comment 8 Michael Nordman 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.
Comment 9 Michael Nordman 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.