Bug 32374 - marquee with display:inline causes crash
Summary: marquee with display:inline causes crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-10 04:44 PST by Shinichiro Hamaji
Modified: 2009-12-17 21:42 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (5.93 KB, patch)
2009-12-10 04:46 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v2 (6.00 KB, patch)
2009-12-11 17:43 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v3 (5.89 KB, patch)
2009-12-13 23:52 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v4 (5.25 KB, patch)
2009-12-17 02:39 PST, Shinichiro Hamaji
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinichiro Hamaji 2009-12-10 04:44:34 PST
The following HTML makes WebKit crash.

<body>
  <div style="opacity: 0.9;">
    <marquee style="opacity: 0.9; display: inline;">No crash means PASS</marquee>
  </div>
</body>

This happens because the marquee code assumes RenderLayer's renderer is a box.
Comment 1 Shinichiro Hamaji 2009-12-10 04:46:30 PST
Created attachment 44606 [details]
Patch v1
Comment 2 WebKit Review Bot 2009-12-10 04:50:19 PST
style-queue ran check-webkit-style on attachment 44606 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-12-11 15:51:42 PST
Comment on attachment 44606 [details]
Patch v1

renderMarquee() could be const, no?

I expect there is lots of code which uses layer()->renderBox().  I wonder how much of the rest of it is wrong.
Comment 4 Shinichiro Hamaji 2009-12-11 17:43:29 PST
Created attachment 44719 [details]
Patch v2
Comment 5 WebKit Review Bot 2009-12-11 17:44:10 PST
style-queue ran check-webkit-style on attachment 44719 [details] without any errors.
Comment 6 Shinichiro Hamaji 2009-12-11 17:49:55 PST
> renderMarquee() could be const, no?

Fixed, thanks.

> I expect there is lots of code which uses layer()->renderBox().  I wonder how
> much of the rest of it is wrong.

Yeah, I think there would be some other crashes, but I couldn't find other crashes so far. I'm guessing toRenderBox in RenderLayer::calculate{,Clip}Rects can cause crashes, but I'm not sure.
Comment 7 Shinichiro Hamaji 2009-12-13 23:52:04 PST
Created attachment 44777 [details]
Patch v3
Comment 8 Shinichiro Hamaji 2009-12-13 23:52:41 PST
Fixed a wrong changelog diff.
Comment 9 WebKit Review Bot 2009-12-13 23:55:57 PST
style-queue ran check-webkit-style on attachment 44777 [details] without any errors.
Comment 10 mitz 2009-12-15 09:07:20 PST
Comment on attachment 44777 [details]
Patch v3

I don’t think this is correct. You should just not create a RenderMarquee for an inline. Just change the condition in RenderLayer::styleChanged() to also check that the renderer is a box.
Comment 11 Shinichiro Hamaji 2009-12-17 02:39:52 PST
Created attachment 45047 [details]
Patch v4
Comment 12 WebKit Review Bot 2009-12-17 02:40:10 PST
style-queue ran check-webkit-style on attachment 45047 [details] without any errors.
Comment 13 Shinichiro Hamaji 2009-12-17 02:47:07 PST
Ah, I see. I was unsure if marquee will never be inline. Thanks! I think we still need the fix for HTMLMarqueeElement as the code assumes it has box renderer.
Comment 14 mitz 2009-12-17 07:37:07 PST
Comment on attachment 45047 [details]
Patch v4

> +RenderMarquee* HTMLMarqueeElement::renderMarquee() const
> +{
> +    if (renderer() && renderer()->hasLayer() && renderBoxModelObject()->layer()->marquee())
> +        return renderBoxModelObject()->layer()->marquee();
> +    return 0;

You don’t need the “&& renderBoxModelObject()->layer()->marquee()” condition.
Comment 15 Shinichiro Hamaji 2009-12-17 21:42:41 PST
Committed r52299: <http://trac.webkit.org/changeset/52299>