Summary: | TOT REGRESSION: Assertion failure loading acid2 test in -[WebCoreFrameBridge installInFrame:] | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||||||||
Component: | New Bugs | Assignee: | Trey Matteson <trey> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Critical | CC: | alice.barraclough, ap, darin, trey | ||||||||||
Priority: | P1 | Keywords: | InRadar, Regression | ||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
URL: | http://webstandards.org/act/acid2/test.html#top | ||||||||||||
Attachments: |
|
Description
Geoffrey Garen
2006-03-12 10:35:53 PST
This seems to mean that the owning renderer is gone for this subframe, but the load is continuing. I tried putting a call to m_frame->stopLoading() inside RenderPart::setFrame(), and that did not help a bit. Maciej points out that the loading should probably be triggered on the DOM side rather than the render side anyway; that's something to fix in the future but may not be important to this bug. I was taking a look at this one. When we decide to render the fallback content, we detach the RenderPart. It seems to me that the flaw here is that this causes us to disconnect the ownerRenderer of the frame. I'm half-guessing that the ownerRenderer is supposed to be the renderer of the owner/parent. If so, detaching the RenderPart doesn't change the frame's parent or its renderer, so why are we calling disconnectOwnerRenderer? Symmetrically, it seems like the ownerRenderer->setFrame(this) in the Frame constructor would be wrong too. Shouldn't the ownerRenderer's frame be the parent frame of the one being constructed? If all this speculation is off the mark, perhaps it at least indicates some places where some commenting would have value. The resolution of my confusion is that the ownerRenderer is the renderer of the *element* within the parent frame which hosts the child frame. Created attachment 7380 [details]
propective patch, for discussion
I now get why darin tried calling frame.stopLoading(), but that is ineffective because most or all the places we stop loading now are driven from WebKit. Calling stopLoading on the WebCore side doesn't inform WK, so it goes on ahead posting data to us later, at which point we hit the assert.
Taking the approach that WK is in charge, I tried the attached change, where we cancel the loading in WK at the point we ask for the fallback. I'm not sure this is the best solution, but it does seem to fix the misbehavior.
Comment on attachment 7380 [details]
propective patch, for discussion
Poster wants the patch reviewed.
Actually this isn't ready for review, as I need to do more testing, and there is no change log. I just attached that patch so I could discuss it with mjs easily on #webkit. I will nominate a proper patch soon. Created attachment 7400 [details]
oops, bogus patch, pls ignore
Created attachment 7402 [details]
proposed patch, for review
Created attachment 7403 [details]
additional patch for fixing Acid2 test case
I'm attaching an additional patch that fixes our local Acid2 test so that it would catch this bug. In addition to the change in the patch, the Acid2 test files must be moved to the http test directory.
BTW, in case you find it more convenient, the only change to the tests is replacing "file://localhost/almost/certainly/nonexistent/file.html" with "/404-dir-should-not-exist/".
Alexey is familiar with this change and said he would land it.
Comment on attachment 7403 [details]
additional patch for fixing Acid2 test case
The change looks perfectly fine, but I cannot grant a formal review; reassigning the request to Maciej.
Comment on attachment 7402 [details]
proposed patch, for review
r=me
Comment on attachment 7403 [details]
additional patch for fixing Acid2 test case
I suggest we keep both copies just so we have a version of it in fast/css.
r=me otherwise
|