Bug 132906

Summary: [CSS Regions] Add ASSERT to make sure using the flowThread cache does not return incorrect results
Product: WebKit Reporter: Radu Stavila <stavila>
Component: CSSAssignee: Radu Stavila <stavila>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kondapallykalyan, WebkitBugTracker
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 132913    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
none
Patch which applies only to regions
simon.fraser: review+
Patch for landing none

Description Radu Stavila 2014-05-14 07:17:41 PDT
[CSS Regions] Add ASSERT to make sure using the flowThread cache does not return incorrect results
Comment 1 Radu Stavila 2014-05-14 07:23:57 PDT
Created attachment 231448 [details]
Patch
Comment 2 Andrei Bucur 2014-05-14 07:31:49 PDT
Comment on attachment 231448 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2014-05-14 08:39:04 PDT
Comment on attachment 231448 [details]
Patch

Clearing flags on attachment: 231448

Committed r168837: <http://trac.webkit.org/changeset/168837>
Comment 4 WebKit Commit Bot 2014-05-14 08:39:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 WebKit Commit Bot 2014-05-14 10:24:24 PDT
Re-opened since this is blocked by bug 132913
Comment 6 Alexey Proskuryakov 2014-05-14 10:26:19 PDT
The assert fires on 6 tests, let's roll out?

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r168837%20(4654)/results.html
Comment 7 Radu Stavila 2014-05-15 04:31:44 PDT
Created attachment 231499 [details]
Patch which applies only to regions

Applied assertion only for regions, as multicol still has some issues. Created https://bugs.webkit.org/show_bug.cgi?id=132946 for multicol.
Comment 8 Simon Fraser (smfr) 2014-05-15 12:21:09 PDT
Comment on attachment 231499 [details]
Patch which applies only to regions

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

> Source/WebCore/rendering/RenderObject.cpp:550
> +            // Failing tests when enabling this for multicol:
> +            // http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r168837%20(4654)/results.html

I don't think it's useful to put this URL in a comment. Eventually the test results will be purged from the server. This kind of info should go in the bug.

> Source/WebCore/rendering/RenderObject.cpp:552
> +            ASSERT(flowThread == locateFlowThreadContainingBlockNoCache());

This if() block only contains an assertion. It would be better to make the "if" test part of the assertion so it's compiled out in release.

> Source/WebCore/rendering/RenderObject.h:893
> +    // This will walk the tree to find the flow thread.

Comment seems to be inaccurate if you're in layout.
Comment 9 Radu Stavila 2014-05-16 09:23:16 PDT
Created attachment 231577 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2014-05-16 11:26:53 PDT
Comment on attachment 231577 [details]
Patch for landing

Clearing flags on attachment: 231577

Committed r168971: <http://trac.webkit.org/changeset/168971>