Summary: | assert-fail in WebCore::FrameLoader::saveDocumentState (document is null) | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jasper Bryant-Greene <jasper> | ||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | jchaffraix, jmalonzo, xan.lopez | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
URL: | http://acid3.acidtests.org/reference.html | ||||||
Bug Depends on: | 18411 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Jasper Bryant-Greene
2008-03-25 03:34:29 PDT
Created attachment 20080 [details] remove bogus assertion I'm pretty sure the ASSERT is bogus, because a frame can contain no document under certain circumstances, and because there's an if() test immediately after that protects against the case that the ASSERT is checking for. Also because although the ASSERT trips on the debug build (for example when clicking the reference rendering link on http://acid3.acidtests.org/), the release build works fine in the same circumstances. Thus this patch removes the ASSERT. (In reply to comment #1) > Created an attachment (id=20080) [edit] > remove bogus assertion > > I'm pretty sure the ASSERT is bogus, because a frame can contain no document > under certain circumstances, and because there's an if() test immediately after > that protects against the case that the ASSERT is checking for. I would be more cautious about removing the ASSERT because first it is never triggered on the Mac port only on Qt/Gtk (as mentionned in http://bugs.webkit.org/show_bug.cgi?id=17984). You should also be carefull as ASSERTs are not enforced on release build so the null check is a necessity for those cases. Finally in bug17984, I showed another way of resolving the bug that does not involve removing the ASSERT (but may lead to memory leaks). > > Also because although the ASSERT trips on the debug build (for example when > clicking the reference rendering link on http://acid3.acidtests.org/), the > release build works fine in the same circumstances. > > Thus this patch removes the ASSERT. > Your patch lacks a test case (it is pretty straightforward as the assertion can be triggered by a page with an iframe and just reloading it). *** Bug 17984 has been marked as a duplicate of this bug. *** Comment on attachment 20080 [details]
remove bogus assertion
r=me
Comment on attachment 20080 [details]
remove bogus assertion
Clearing review flag because I think Julien is right. The assertion is correct and in fact it's the null check after the assertion that's wrong We should fix the bug instead.
Jasper, the saveDocumentState does try to save frame's information but it is pointless to save frame's information when they are not connected to a Document. The core of the issue is how the frames are released. That's why the gtk port is the only one concerned with this bug. I have said that the Qt port would also fail but it is not longer the case. I could not find exactly when it was fixed. If you want to attack the real issue, I would advise you to have a look at http://trac.webkit.org/projects/webkit/changeset/31521. This is the changeset where Zecke has moved the portion of the code I had hold responsible for the assertion failure (the code in FrameLoaderClient::detachedFromParent4()). The other Qt commits from the same day should give an answer to how and when the frame should be released to avoid both the assertion and a memory leak. *** Bug 18334 has been marked as a duplicate of this bug. *** I thought Frames were always created with "Empty Documents"? Maciej went through significant pains to accomplish that for us, and we spent time cleaning up all the loose ends, but are better off for it. Maybe that empty document creation has pieces reaching into WebKit code and thats why Win/Mac are fine but not Qt/GTK? The patch at bug #18411 fixes this issue so i'll mark this as dependent on that bug. Feel free to remove the dependency if you think otherwise. (In reply to comment #9) > The patch at bug #18411 fixes this issue so i'll mark this as dependent on that > bug. Feel free to remove the dependency if you think otherwise. > This bug seems to have been fixed by the patch at #18411. Closing as FIXED. |