| Summary: | ASSERTION FAILED in WebCore::RenderFlowThread::getRegionRangeForBox | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Renata Hodovan <rhodovan.u-szeged> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | Andrei Bucur <abucur> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | abucur, commit-queue, esprehn+autocc, glenn, hyatt, kondapallykalyan, mihnea, simon.fraser, WebkitBugTracker | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | 132946 | ||||||||||||||
| Bug Blocks: | 116980 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Renata Hodovan
2014-08-04 05:48:00 PDT
If you run the attached test with a release build then it causes a crash in WebCore::RenderFlowThread::getRegionRangeForBox. Created attachment 239476 [details]
WIP Patch
Comment on attachment 239476 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239476&action=review > Source/WebCore/rendering/LayoutState.h:130 > + RenderFlowThread* m_currentRenderFlowThread; There’s a new idiom we can and should use for data member initialization now that we use new enough compilers. We would write: RenderFlowThread* m_currentRenderFlowThread { nullptr }; And then we would not need to change the various constructors to initialize this data member. > Source/WebCore/rendering/RenderBlock.cpp:133 > + RenderFlowThread* m_flowThreadContainingBlock; > + bool m_dirtyFlowThreadContainingBlock; RenderFlowThread* m_flowThreadContainingBlock { nullptr }; bool m_dirtyFlowThreadContainingBlock { true }; I find the m_dirtyFlowThreadContainingBlock data member name confusing. In fact, the same name is used below as a verb phrase. Maybe m_flowThreadContainingBlockNeedsUpdate, or reverse the sense and make it m_flowThreadContainingBlockIsValid? Or maybe use Optional<RenderFlowThread*> instead of a separate data member. > Source/WebCore/rendering/RenderBlock.cpp:3346 > + if (this->isRenderFlowThread()) No need for this-> here. > Source/WebCore/rendering/RenderBlock.cpp:3351 > + RenderBlockRareData* rareData = getRareData(this); > + if (!rareData) > + rareData = &ensureRareData(this); This should just be: RenderBlockRareData& rareData = ensureRareData(this); > Source/WebCore/rendering/RenderBlock.cpp:3361 > + RenderBlockRareData* rareData = getRareData(this); > + if (!rareData) > + rareData = &ensureRareData(this); This should just be: RenderBlockRareData& rareData = ensureRareData(this); > Source/WebCore/rendering/RenderBlock.cpp:3369 > +RenderFlowThread* RenderBlock::locateFlowThreadContainingBlock() const It’s not the best style to use const and const_cast to account for the side effect. Instead we would normally either make the function non-const to make the side effect clear, or alternatively make the relevant data members mutable and make updateFlowThreadContainingBlock also be const. > Source/WebCore/rendering/RenderBlock.h:317 > + RenderFlowThread* updateFlowThreadContainingBlock(RenderFlowThread*); > + void dirtyFlowThreadContainingBlock(); > + RenderFlowThread* cachedFlowThreadContainingBlock() const; Do all three of these need to be public? Can they be private or protected instead? I don’t think the name “dirtyFlowThreadContainingBlock” makes it clear that “dirty” is a verb. Normally we use different naming for such things, following style from Cocoa. For example, setFlowThreadContainingBlockNeedsUpdate. > Source/WebCore/rendering/RenderObject.cpp:2019 > + dirtyFlowThreadContainingBlockRecursive(); I prefer a name that reflects what is being done, not the algorithm. The name “recursive” is not entirely clear to me. Does it mean this call will dirty all descendants? All ancestors? > Source/WebCore/rendering/RenderObject.cpp:2063 > + RenderBlock* block = toRenderBlock(this); > + block->dirtyFlowThreadContainingBlock(); It should be: toRenderBlock(*this).dirtyFlowThreadContainingBlock(); We should use a reference rather than a pointer. > Source/WebCore/rendering/RenderObject.cpp:2072 > + for (RenderObject* child = firstChildSlow(); child; child = child->nextSibling()) > + child->dirtyFlowThreadContainingBlockRecursive(); It’s very strange to use firstChildSlow here. But also, it would be better to use a non-recursive algorithm. Here’s one way to write it: for (RenderObject* descendant = firstChild(); descendant; descendant = descendant->nextInPreOrder(this)) descendant->dirtyFlowThreadContainingBlock(); Then we use a loop instead of a recursive function call and we don’t have to use stack space proportional to the size of the tree. > Source/WebCore/rendering/RenderObject.cpp:2076 > + if (!isRenderBlock()) { Code like this makes me wonder why we aren’t using a virtual function. > Source/WebCore/rendering/RenderObject.cpp:2077 > + RenderBlock* cb = containingBlock(); Please don’t abbreviate to cb. Instead write something more like this: if (auto* containingBlock = this->containingBlock()) flowThread = containingBlock->cachedFlowThreadContainingBlock(); > Source/WebCore/rendering/RenderObject.cpp:2081 > + RenderBlock* block = toRenderBlock(this); Please use a reference instead of a pointer here. > Source/WebCore/rendering/RenderObject.cpp:2619 > + RenderBlock* cb = containingBlock(); Please don’t use the name “cb” in new code: RenderBlock* containingBlock = this->containingBlock(); > Source/WebCore/rendering/RenderObject.h:222 > + RenderFlowThread* flowThreadContainingBlock() const; What’s behind the decision to not make this inline any more? Comment on attachment 239476 [details]
WIP Patch
So I think the only time a positioned object is looked at when the wrong flow thread is on the stack is for static position computation, which involves doing stuff with positioned objects in the normal flow. I have no real opinion on the change and think it's probably fine, but if you were looking to do something smaller, you could probably just pop the flow thread while examining the positioned object from normal flow layout, and then push the thread back on the stack when done.
@Darin Thanks for the feedback and for the useful information about the Optional template class. I'll update the patch if we decide to go down this road. Here are the performance results for Regions and html5-full-render. The first column HAS the patch, the second column doesn't have the patch. /Layout/RegionsAuto:Time ms 300.90 2.75% Worse 292.85 /Layout/RegionsAutoMaxHeight:Time ms 1070.55 3.45% Worse 1034.85 /Layout/RegionsExtendingSelectionMixedContent:Time ms 2017.85 6.15% Better 2150.00 /Layout/RegionsFixed:Time ms 935.85 4.26% Worse 897.60 /Layout/RegionsFixedShort:Time ms 1136.80 3.58% Worse 1097.50 /Layout/RegionsSelectAllMixedContent:Time ms 697.35 1.04% Better 704.65 /Layout/RegionsSelection:Time ms 1538.70 8.20% Better 1676.10 /Layout/RegionsShapes:Time ms 418.45 2.83% Worse 406.95 /Parser/html5-full-render:Time ms 3888.90 3853.65 There is a good performance gain in selection tests. However, there's a slight performance degradation for the other tests. I'll run some numbers for multi-col as well. Here are the performance results when running the tests from https://bugs.webkit.org/show_bug.cgi?id=137687 : /Layout/Multicol/MulticolManyColumns:Time ms 7130.25 ± 0.05% 1.80% Better 7260.85 ± 0.45% /Layout/Multicol/MulticolNested:Time ms 4604.70 ± 0.17% 5.01% Better 4847.50 ± 0.11% The build using this approach proves to be 1.8% faster on a test with many columns and 5.01% faster on a test with many nested multi-column containers. Even though the performance for regions drops a bit I see the following advantages for moving forward with this approach: 1. The multi-column implementation benefits from this. 2. The selection code and anything not using currentRenderFlowThread benefits from this. 3. We can drop the CurrentRenderFlowThreadMaintainer/Disabler classes. During the development of this patch I've discovered two places that needed CurrentRenderFlowThreadDisabler but didn't have it. The downside is the regions code is a bit slower. I believe we can regain the lost performance in those tests by optimizing the repaint code which is the major bottleneck in those tests (each test has around 1200 regions and repainting the flow thread actually means calling repaint() 1200 times). Created attachment 239800 [details]
Patch
Created attachment 239803 [details]
Patch v2
Comment on attachment 239803 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=239803&action=review Seems ok. If you're trying to catch all things that can change your containing block, I would actually eyeball the containingBlock() function. transforms can do it in addition to positioning for example. > Source/WebCore/rendering/RenderBlock.cpp:319 > + if (oldStyle && oldStyle->position() != newStyle.position()) > + invalidateFlowThreadContainingBlockIncludingDescendants(); transforms can change your containing block too. You may want to include that? Created attachment 239867 [details]
Patch v3
I've changed some things in the patch: - I've refactored and renamed RenderObject::removeFromRenderFlowThreadRecursive . Now it doesn't invalidate the flow thread state flag if the function descends inside a nested flow thread. - Changing the transform state of a block also invalidates the flowThreadContainingBlock chain - There's a new test verifying this works correctly and doesn't crash Comment on attachment 239867 [details]
Patch v3
r=me
Comment on attachment 239867 [details] Patch v3 Clearing flags on attachment: 239867 Committed r174761: <http://trac.webkit.org/changeset/174761> All reviewed patches have been landed. Closing bug. |