Summary: | Assertion failure in WebCore::RenderHTMLCanvas::layout | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Rowe (bdash) <mrowe> | ||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Major | CC: | darin, eric, hamaji | ||||||||||||
Priority: | P2 | Keywords: | HasReduction | ||||||||||||
Version: | 420+ | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | OS X 10.4 | ||||||||||||||
Attachments: |
|
Description
Mark Rowe (bdash)
2006-12-31 16:32:08 PST
This looks very similar to bug 12044 and bug 12041. One difference is that I could not cause this to crash with a canvas element when it was the last element inside the body. Created attachment 34730 [details]
Patch v1
---
5 files changed, 64 insertions(+), 2 deletions(-)
> Created an attachment (id=34730) [details]
Now, I don't see the original ASSERTion Mark reported but this HTML seems to hit another ASSERTion. Also, if a run-in video tag has control, safari crashes even if it is release mode build.
Though I'm not sure if this is the best way (I don't know how non-block run-in should be handled), this patch should fix the crash.
Comment on attachment 34730 [details] Patch v1 review- because of the double change log in the LayoutTests directory. > + // block. We also don't handle the run-in if the element isn't a > + // block (e.g., <canvas>, <video>, and etc.). > + if (!child->isRunIn() || !child->childrenInline() && !child->isReplaced() || !child->isRenderBlock()) > return false; This definitely gets rid of the assertion/crash, but I am not 100% certain it's correct. I'd like to hear from Hyatt or Dan Bernstein. Comment on attachment 34730 [details]
Patch v1
It seems like replaced elements could be display:run-in and actually be made to work... I am curious what Mozilla does.
Thanks Darin and David for your comments. It seems that Firefox (3.5.2) doesn't handle replaced run-in properly. If I understand correctly, browsers should layout <canvas style="display:run-in;"></canvas> <p>foo</p> just like <p><canvas style="display:inline"></canvas>foo</p> With the following test case, firefox is treating the run-in canvas just like a block element: http://tinyurl.com/qbodd6 Maybe it's OK to commit a patch which fixes the crash bug with incorrect replaced run-in handling and a FIXME comment for now? Created attachment 34785 [details]
Patch v2
---
5 files changed, 53 insertions(+), 1 deletions(-)
Comment on attachment 34785 [details]
Patch v2
Are replaced renderers that can show up here that are not a RenderBlock?
(In reply to comment #8) > Are replaced renderers that can show up here that are not a RenderBlock? I meant to say: Are replaced renderers the only kind that can show up here that are not a RenderBlock? > Are replaced renderers the only kind that can show up here that are not a
> RenderBlock?
I think so... But yeah, I'm not 100% sure and people may add renderer which passes this check. I'll upload another patch. Thanks for this catch!
Created attachment 34788 [details]
Patch v3
---
5 files changed, 53 insertions(+), 1 deletions(-)
Comment on attachment 34788 [details]
Patch v3
Style:
if (window.layoutTestController) {
17 layoutTestController.dumpAsText();
18 }
Created attachment 34801 [details]
Patch v4
---
5 files changed, 52 insertions(+), 1 deletions(-)
> Style:
> if (window.layoutTestController) {
> 17 layoutTestController.dumpAsText();
> 18 }
Ah, I shouldn't have brackets for single-line if, right? Fixed.
Comment on attachment 34801 [details]
Patch v4
Shinichiro doesn't have hit commit bit yet, so adding cq+.
Comment on attachment 34801 [details] Patch v4 Clearing flags on attachment: 34801 Committed r47400: <http://trac.webkit.org/changeset/47400> All reviewed patches have been landed. Closing bug. I think this caused the Tiger and Qt bots to show failures. Re-opening. Created attachment 35018 [details]
Qt test fix
---
2 files changed, 13 insertions(+), 1 deletions(-)
(In reply to comment #18) > I think this caused the Tiger and Qt bots to show failures. Re-opening. Sorry for this. I believe the patch fixes the test failure. As Qt doesn't support <video>, the string "foo" in <video> is rendered and it makes the test fail. I think we can just remove the string. The main point of this test is just checking if the webcore crashes or not and we don't need to test availability of video tag. Comment on attachment 35018 [details]
Qt test fix
LGTM.
Comment on attachment 35018 [details]
Qt test fix
Rejecting patch 35018 from commit-queue. This patch will require manual commit.
['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment on attachment 35018 [details]
Qt test fix
Testing 11087 test cases.
fast/profiler/call.html -> crashed
Exiting early after 1 failures. 7375 tests run.
Was the failure. Unlikely that was related. Adding this back to the commit-queue.
Comment on attachment 35018 [details] Qt test fix Clearing flags on attachment: 35018 Committed r47421: <http://trac.webkit.org/changeset/47421> All reviewed patches have been landed. Closing bug. |