Bug 184951 - [LFC] Implement LayoutContexet::layout() and its dependencies.
Summary: [LFC] Implement LayoutContexet::layout() and its dependencies.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-24 19:41 PDT by zalan
Modified: 2018-04-25 15:59 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.38 KB, patch)
2018-04-25 14:39 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (18.38 KB, patch)
2018-04-25 15:20 PDT, zalan
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2018-04-24 19:41:05 PDT
LFC -> Layout Formatting Context
Comment 1 zalan 2018-04-25 14:39:12 PDT
Created attachment 338796 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-04-25 14:43:22 PDT
Comment on attachment 338796 [details]
Patch

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

> Source/WebCore/layout/FormattingContext.h:50
> +    const Box& root() const { return *m_root; }

You can't return a ref to a weak object. It might be null!

Can't we guarantee that the FormattingContext outlines the root and not use a WeakPtr?

> Source/WebCore/layout/LayoutContext.cpp:68
> +    if (formattingContextRoot.establishesBlockFormattingContext())
> +        return std::make_unique<BlockFormattingContext>(formattingContextRoot);
> +    if (formattingContextRoot.establishesInlineFormattingContext())
> +        return std::make_unique<InlineFormattingContext>(formattingContextRoot);
> +    ASSERT_NOT_REACHED();

I would prefer some blank lines after the returns.

> Source/WebCore/layout/LayoutContext.h:53
> +    LayoutContext(Box& root);

Can this be a const&?
Comment 3 zalan 2018-04-25 15:12:19 PDT
(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 338796 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338796&action=review
> 
> > Source/WebCore/layout/FormattingContext.h:50
> > +    const Box& root() const { return *m_root; }
> 
> You can't return a ref to a weak object. It might be null!
In normal conditions, this can't be null (formatting context should never outlive the box). In unexpected conditions, it is just a null deref (as opposed to UAF).
I could return Box*, but that would indicate that it's okay to have a null root and callers would start checking against it.

> 
> Can't we guarantee that the FormattingContext outlines the root and not use
> a WeakPtr?
I don't think we can guarantee that (we could release assert on the m_root at best). 

> 
> > Source/WebCore/layout/LayoutContext.cpp:68
> > +    if (formattingContextRoot.establishesBlockFormattingContext())
> > +        return std::make_unique<BlockFormattingContext>(formattingContextRoot);
> > +    if (formattingContextRoot.establishesInlineFormattingContext())
> > +        return std::make_unique<InlineFormattingContext>(formattingContextRoot);
> > +    ASSERT_NOT_REACHED();
> 
> I would prefer some blank lines after the returns.
ok.

> 
> > Source/WebCore/layout/LayoutContext.h:53
> > +    LayoutContext(Box& root);
> 
> Can this be a const&?
Not yet. WeakPtr can't take const objects.
Comment 4 zalan 2018-04-25 15:20:34 PDT
Created attachment 338808 [details]
Patch
Comment 5 zalan 2018-04-25 15:54:08 PDT
Committed r231028: <https://trac.webkit.org/changeset/231028>
Comment 6 Radar WebKit Bug Importer 2018-04-25 15:59:03 PDT
<rdar://problem/39738535>