Summary: | JavascriptURL as frame src crasher. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Nordman <michaeln> | ||||||||
Component: | Frames | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | beidson, darin, fishd, levin, mjs | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Michael Nordman
2009-06-05 18:52:49 PDT
Do you have a test case or instructions on how to reproduce this bug? (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 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. 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.
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.
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?
(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. 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. Created attachment 31066 [details]
Patch with tests
Added a layout test in take 3.
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. (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. CCing loader people. 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.
Landed in @r44541 |