Bug 26230 – JavascriptURL as frame src crasher.

• Component: Frames
• Status: RESOLVED FIXED

[Michael Nordman]

This is a blocking bug for chrome.
See http://code.google.com/p/chromium/issues/detail?id=12161

The incorrect javascript handling is a secondary issue. The blocker is the crasher.

This webkit bug was split out of bug 16855, which is an umbrella for all manner of issues related to javascript urls. I hijacked that umbrella too much for this particular issue already, so have opened this new more focused bug. There are comments (and preliminary patches) specific to chrome's issue in bug 16855 that I have not copied here (so please see them there).

As mentioned in the comments over there, I'm not so sure about the extra call to stopLoader(false) in FrameLoader::executeIfJavaScriptURL. What seems like is really missing is a call to cache()->loader()->cancelRequests(docLoader) for the document being torn down in the act of replacing the document. The call to stopLoader(false) gets that done and much much more. I'm concerned about what side effects the much much more may have (although all layout tests do pass).

I took a few other stabs at it.

1) In DocumentLoader::stopLoading() call cancelRequests(). The method comments sound like this would be appropiate for that method.
// Cancels the data source's pending loads.  Conceptually, a data source only loads
// one document at a time, but one document may have many related resources. 
// stopLoading will stop all loads initiated by the data source, 
// but not loads initiated by child frames' data sources -- that's the WebFrame's job.
But... a handful of layout tests fail as a result.

2) In FrameLoader::executeIfJavaScriptURL() replace the call to stopLoading(false) with the more focused call to cancelRequests(). This alone resolves the very specific crashing bug that I'm seeing with javascript urls that construct the document programatically.

I'm concerned there are other scenarios where the Document/DocLoader will get blown away while requests are still pending. We're seeing a number of stack traces that indicate that has occurred sometime after the fact. I think its unlikely that javascript urls in frameset documents are responsible for all of them. We think differences in protective refcounting between jsc and v8 bindings and or differences in gc strategies may be involved.

Given the uncertainty, and the sevirity (CRASH!!!)... i took another stab at it that casts a wider net.

3) In DocLoader::~DocLoader(), dont just ASSERT(!m_requestCount), proactively cancel them... behind a PLATFORM(CHROMIUM) flag. This should not affect other platforms as they should never have active requests at dtor time (per the existing assertion).

I'm leaning towards option 3 (for m3) unless we can determine a better fix real soon now.

[Mark Rowe (bdash)]

Do you have a test case or instructions on how to reproduce this bug?

[Michael Nordman]

(In reply to comment #1)
> Do you have a test case or instructions on how to reproduce this bug?

Yes, see comment #9 in http://code.google.com/p/chromium/issues/detail?id=12161

[David Levin]

Independently, I was thinking that #3 is the best thing to do here (without the ifdef and remove the assert -- just do if (m_requestCount) cancelLoads....

The assert was added in: https://bugs.webkit.org/show_bug.cgi?id=23736

So far the fixes for that assert have been to call stop loader in various places in FrameLoader.  So the fixes have only stopped the loading in a different place, but in ~DocLoader is the most robust place to stop the loading.

[Michael Nordman]

Created attachment 31055 [details]
Cancel requests in ~DocLoader

Patch for option 3 as outlined above. I left the ASSERT in as well, after the call to cancel requests.

[Michael Nordman]

Created attachment 31056 [details]
Cancel requests in ~DocLoader (take2)

Only call cancelRequests if the loader has pending requests as that method can be somewhat expensive.

[Eric Seidel (no email)]

Comment on attachment 31056 [details]
Cancel requests in ~DocLoader (take2)

Your patch needs to include a test case.  Even if that test case only crashes in Chromium.  Your ChangeLog (or at least the bug) should document under which conditions this crash occurs.  From reading the bug it seems it only occurs in Chromium?

[Michael Nordman]

(In reply to comment #6)
>  From reading the bug it seems it only occurs in Chromium?

I can't speak to other platforms (although per david's comments it seems there have been patches creeping in to cancel these pending requests at other callsites).

I also don't know all of the circumstances in which chrome may trip on this. The one I know of for certain is the javascript url that constructs a document which has some resource loads initiated on its behalf. After that doc is torn down (in the act of replacing it with the simple string), chrome crashes. I think the number of crashes we're seeing indicate there are other circumstances as well.

I didn't know a one line crashing bug fix needed a new test case (as no new functionality is being added). "If no layout test can be (or needs to be) constructed for the fix, you must explain why a new test isn't necessary to the reviewer." Is it not readily apparent that pending requests at DocLoader dtor time are a recipe for crashes?

Will add one, I haven't added a layout-test yet (ever), may take me some hunting around to figure out how to plug what in where.

[Michael Nordman]

I think i've found the right place for a test...

fast/loader/ javascript-url-as-framesrc-crash.html

An odd thing about this test is that the expected results at this time are not really correct. The document generated by the script url should be respected, but webcore doesn't do that yet and this patch doesn't address that correctness issue.

[Michael Nordman]

Created attachment 31066 [details]
Patch with tests

Added a layout test in take 3.

[Michael Nordman]

The expected output in this test will be the same regardless of which of the two possible documents webcore decides to populate the child frame with (simple string return value vs document.written data). So if/when the correctness issues with javascript urls are resolved, this test should still pass.

[Eric Seidel (no email)]

(In reply to comment #7)
> I didn't know a one line crashing bug fix needed a new test case (as no new
> functionality is being added). "If no layout test can be (or needs to be)
> constructed for the fix, you must explain why a new test isn't necessary to the
> reviewer." Is it not readily apparent that pending requests at DocLoader dtor
> time are a recipe for crashes?

Of course crashing fixes need test cases. :)  I guess we should fix our docs to make that more clear.  We don't want to start crashing again in the future.  If you can't make a test case for the crash fix, then we don't add one... but in this case, we can.

[Eric Seidel (no email)]

CCing loader people.

[Eric Seidel (no email)]

Comment on attachment 31066 [details]
Patch with tests

The question which remains to be answered is if this is the right place to do this cancelRequests.  Clearly either the destructor should ASSERT(!m_requestCount) or it should cancel any pending requests.

I think this looks OK to me.  Please yell, any of you CC'd loader folks, if you disagree.

[Brent Fulgham]

Landed in @r44541