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

Radu Stavila
Reported 2014-05-14 07:17:41 PDT
[CSS Regions] Add ASSERT to make sure using the flowThread cache does not return incorrect results
Attachments
Patch (3.51 KB, patch)
2014-05-14 07:23 PDT, Radu Stavila 		no flags
DetailsFormatted DiffDiff
Patch which applies only to regions (4.10 KB, patch)
2014-05-15 04:31 PDT, Radu Stavila 		simon.fraser: review+
DetailsFormatted DiffDiff
Patch for landing (3.82 KB, patch)
2014-05-16 09:23 PDT, Radu Stavila 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radu Stavila
Comment 1 2014-05-14 07:23:57 PDT
Created attachment 231448 [details] Patch
Andrei Bucur
Comment 2 2014-05-14 07:31:49 PDT
Comment on attachment 231448 [details] Patch r=me
WebKit Commit Bot
Comment 3 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>
WebKit Commit Bot
Comment 4 2014-05-14 08:39:07 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 5 2014-05-14 10:24:24 PDT
Re-opened since this is blocked by bug 132913
Alexey Proskuryakov
Comment 6 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
Radu Stavila
Comment 7 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.
Simon Fraser (smfr)
Comment 8 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.
Radu Stavila
Comment 9 2014-05-16 09:23:16 PDT
Created attachment 231577 [details] Patch for landing
WebKit Commit Bot
Comment 10 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>
Note You need to log in before you can comment on or make changes to this bug.