Bug 206610 – Crash in RenderBlockFlow::adjustLinePositionForPagination() with complex line without root box

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Doug Kelly]

If RenderBlockFlow::adjustLinePositionForPagination() encounters a line with no root box, the result will be a crash with a null pointer dereference.

[Doug Kelly]

<rdar://problem/57073037>

[Doug Kelly]

Created attachment 388472 [details]
Patch

[Sam Weinig]

Comment on attachment 388472 [details]
Patch

Is everything in that test case needed to trigger the crash (there is a lot of craziness in there)? Seems like it can probably be reduced further?

[Doug Kelly]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 388472 [details]
> Patch
> 
> Is everything in that test case needed to trigger the crash (there is a lot
> of craziness in there)? Seems like it can probably be reduced further?

Yeah, I tried to reduce the number of "strange" things in the test case that would still trigger the crash, and this was what I came up with.  If someone more familiar with the code knows a more direct means to trigger the same state/crash, I can't say how happy I'd be to replace this test case. :)

[zalan]

Created attachment 388565 [details]
Test reduction

[zalan]

Comment on attachment 388472 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockFlow.cpp:1813
> +            if (!firstRootBox)
> +                return;

I think what we want here is to ignore the firstRootBox when it is nullptr but still call setPaginationStrut() (not that it matters since this is such an edge case)
(and please use the attached test reduction)

[Sam Weinig]

(In reply to Doug Kelly from comment #4)
> (In reply to Sam Weinig from comment #3)
> > Comment on attachment 388472 [details]
> > Patch
> > 
> > Is everything in that test case needed to trigger the crash (there is a lot
> > of craziness in there)? Seems like it can probably be reduced further?
> 
> Yeah, I tried to reduce the number of "strange" things in the test case that
> would still trigger the crash, and this was what I came up with.  If someone
> more familiar with the code knows a more direct means to trigger the same
> state/crash, I can't say how happy I'd be to replace this test case. :)

Oh drat, looks like Zalan beat me to it. His is good.

[Doug Kelly]

Created attachment 388731 [details]
Patch

[Doug Kelly]

Thanks Zalan/Sam. :) Incorporated the suggestions, and the much simpler test case.

[Darin Adler]

Comment on attachment 388731 [details]
Patch

Would be even better if the test covered correct behavior as well as the lack of a crash.

[Doug Kelly]

(In reply to Darin Adler from comment #10)
> Comment on attachment 388731 [details]
> Patch
> 
> Would be even better if the test covered correct behavior as well as the
> lack of a crash.

I agree — testing the behavior here would be best, but I’ll need help from someone (Sam or Zalan?) to understand what the correct behavior in this case is.  Thanks!

[WebKit Commit Bot]

Comment on attachment 388731 [details]
Patch

Clearing flags on attachment: 388731

Committed r255437: <https://trac.webkit.org/changeset/255437>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.