Bug 68938 - REGRESSION(r95573): Crash when loading SVG documents in a flattened frame or any SVG document in Chromium/Mac.
Summary: REGRESSION(r95573): Crash when loading SVG documents in a flattened frame or ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 68480
  Show dependency tree / graph
 
Reported: 2011-09-27 14:35 PDT by Dimitri Glazkov (Google)
Modified: 2011-09-28 13:54 PDT (History)
2 users (show)

See Also:

Attachments
Patch (4.99 KB, patch)
2011-09-27 14:39 PDT, Dimitri Glazkov (Google) 		no flags Details | Formatted Diff | Diff
Patch (6.37 KB, patch)
2011-09-28 09:30 PDT, Dimitri Glazkov (Google) 		no flags Details | Formatted Diff | Diff
Patch (8.60 KB, patch)
2011-09-28 13:27 PDT, Dimitri Glazkov (Google) 		hyatt: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-09-27 14:35:46 PDT 
[Chromium/Mac] REGRESSION(r95573): All SVG documents crash on loading.
Comment 1 Dimitri Glazkov (Google) 2011-09-27 14:39:55 PDT 
Created attachment 108901 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-09-27 15:55:54 PDT 
Comment on attachment 108901 [details]
Patch

needs more testing.
Comment 3 Dimitri Glazkov (Google) 2011-09-28 09:30:41 PDT 
Created attachment 109028 [details]
Patch
Comment 4 Dave Hyatt 2011-09-28 09:34:43 PDT 
Comment on attachment 109028 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109028&action=review

Really does seem odd that Chromium is calling this on the RenderView... how do you even get the answer you want? I'm not convinced this is even doing what you think it's doing.

> Source/WebCore/rendering/RenderBlock.cpp:4897
> -    LayoutUnit cw = containingBlock()->contentLogicalWidth();
> +    LayoutUnit cw = containingBlockOrSelf(this)->contentLogicalWidth();

I wouldn't do it this way, since it doesn't really make any sense. This value really shouldn't even be used by anything since the RenderView can't have percentage heights/widths specified on it, so null checking should be better.

RenderBlock* cb = containingBlock();
LayoutUnit cw = cb ? cb->contentLogicalWidth() : 0;

I'd prefer that.
Comment 5 Dimitri Glazkov (Google) 2011-09-28 13:27:02 PDT 
Created attachment 109062 [details]
Patch
Comment 6 Dimitri Glazkov (Google) 2011-09-28 13:29:46 PDT 
(In reply to comment #4)
> (From update of attachment 109028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109028&action=review
> 
> Really does seem odd that Chromium is calling this on the RenderView... how do you even get the answer you want? I'm not convinced this is even doing what you think it's doing.

Added test for flattened frames which also causes the same crash.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:4897
> > -    LayoutUnit cw = containingBlock()->contentLogicalWidth();
> > +    LayoutUnit cw = containingBlockOrSelf(this)->contentLogicalWidth();
> 
> I wouldn't do it this way, since it doesn't really make any sense. This value really shouldn't even be used by anything since the RenderView can't have percentage heights/widths specified on it, so null checking should be better.
> 
> RenderBlock* cb = containingBlock();
> LayoutUnit cw = cb ? cb->contentLogicalWidth() : 0;
> 
> I'd prefer that.

Done!
Comment 7 Dave Hyatt 2011-09-28 13:31:39 PDT 
Comment on attachment 109062 [details]
Patch

r=me
Comment 8 Dimitri Glazkov (Google) 2011-09-28 13:54:52 PDT 
Committed r96258: <http://trac.webkit.org/changeset/96258>