WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
26230
JavascriptURL as frame src crasher.
https://bugs.webkit.org/show_bug.cgi?id=26230
Summary
JavascriptURL as frame src crasher.
Michael Nordman
Reported
2009-06-05 18:52:49 PDT
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.
Attachments
Cancel requests in ~DocLoader
(1.09 KB, patch)
2009-06-08 11:21 PDT
,
Michael Nordman
no flags
Details
Formatted Diff
Diff
Cancel requests in ~DocLoader (take2)
(1.12 KB, patch)
2009-06-08 11:36 PDT
,
Michael Nordman
eric
: review-
Details
Formatted Diff
Diff
Patch with tests
(3.33 KB, patch)
2009-06-08 15:19 PDT
,
Michael Nordman
eric
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Mark Rowe (bdash)
Comment 1
2009-06-05 20:51:40 PDT
Do you have a test case or instructions on how to reproduce this bug?
Michael Nordman
Comment 2
2009-06-07 09:09:55 PDT
(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
Comment 3
2009-06-08 00:15:22 PDT
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
Comment 4
2009-06-08 11:21:32 PDT
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
Comment 5
2009-06-08 11:36:24 PDT
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 6
2009-06-08 13:01:57 PDT
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
Comment 7
2009-06-08 13:22:06 PDT
(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
Comment 8
2009-06-08 13:56:36 PDT
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
Comment 9
2009-06-08 15:19:36 PDT
Created
attachment 31066
[details]
Patch with tests Added a layout test in take 3.
Michael Nordman
Comment 10
2009-06-08 16:31:16 PDT
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)
Comment 11
2009-06-08 18:11:02 PDT
(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)
Comment 12
2009-06-08 18:12:21 PDT
CCing loader people.
Eric Seidel (no email)
Comment 13
2009-06-08 18:14:07 PDT
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
Comment 14
2009-06-09 13:03:48 PDT
Landed in @
r44541
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug