15.05 KB, text/html
13.83 KB, patch
|Details | Diff | Splinter Review|
13.58 KB, patch
|Details | Diff | Splinter Review|
+++ This bug was initially created as a clone of Bug #1141919 +++ Follow-up from bug 1141919 comment 36: It happens here: https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7916 The line: > InsertFrames(insertion.mParentFrame, kPrincipalList, prevSibling, frameItems); tries to insert the newly constructed frames into the principal list while the prevSibling actually referes to a frame in the overflow list.
Created attachment 8577611 [details] stack + frame dumps Here's the frame tree before the InsertFrames() call where the error occurs, the call stack, and the frame tree after the call. Note that both OverflowList and PrincipalList are now bogus.
So, from the top of my head, a few alternative fixes: 1. make the frame constructor check which list 'prevSibling' is really on, and insert accordingly 2. make a convenience function for the frame constructor to use that takes an alias for which frame lists to check, e.g. kNormalFlow for Overflow/Principal lists, and deal with updating the lists there 3. introduce kNormalFlow and other aliases generally that "external" callers should use instead of the exact child list and update all frame methods (Insert/Append/RemoveFrame etc) to deal with it Are there other alternatives? I think 1. is not acceptable b/c it's too slow. 2 is fast because it only needs to check mFirstChild/mLastChild on the aliased lists. 2 and 3 would basically do the same checks although 3 does it more generally, 2 is just fixing the frame constructor. I think I'm clearly in favor of 3, although 2 might be a less risky change for branches. 3 is good from an architectural POV, since it's a step towards removing the aChildList param from frame methods and let frame classes deal with it internally as much as possible (it can deduce the _alias_ from the frame type as needed). BTW, other lists like kFloatList/kPushedFloatsList etc might potentially have the same problem. We need to audit all frame list uses in the frame constructor for this.
Looks good. I guess it'd be better to check whether the prev frame's next sibling is nullptr first, then principal's last, then overflow's last, and trigger a release crash if it doesn't match any of them.
That's a good suggestion, thanks.
Created attachment 8577807 [details] [diff] [review] wip, alt 4. Alt 4. DWIM! It seems unnecessary to introduce new aliases when we could just do what the caller intended anyway, i.e. for kPrincipalList, try also kOverflowList. For kFloatList, try also kPushedFloatsList etc. As we already do in RemoveFrame for example. The code in this patch is a bit boilerplate-ish, I should probably move it to nsFrameList.h with an API similar to Start/ContinueRemoveFrame.
Any updates on a final patch so we can get this into the 38 release?
Created attachment 8590476 [details] [diff] [review] fix I implemented a StartInsertFrames/ContinueInsertFrames approach first, but I didn't quite like the complexity, and the fact that for floats/pushedFloats you also need to set frame bits on the child frames etc. So, it's simpler and more robust to just drain the overflow list before doing the insertion, which we already have code for. It's a virtual call and a frame property lookup which I hope overall will be a negligible part of inserting content. It's expected that the overflow list is empty, so the more expensive code (the reparenting that nsInlineFrame does) shouldn't run normally. Patch overview: for nsBlockFrame: * remove the unused aState from nsBlockFrame::DrainPushedFloats * spawn off the first part of that method into DrainSelfPushedFloats * calls DrainSelfPushedFloats before inserting/appending a float (nsBlockFrame already deals with principal/overflow lists for normal flow) for other frame types: * call DrainSelfOverflowList() where needed Not tested on Try yet. I did confirm that it fixes the crash in bug 1141919 *without* Xidorn's fixes there.
Comment on attachment 8590476 [details] [diff] [review] fix [Security approval request comment] How easily could an exploit be constructed based on the patch? Very hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? All. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I expect backporting to be easy. How likely is this patch to cause regressions; how much testing does it need? Low risk, this code is well covered by tests so if there's any problem with the patch it should show up in the first test run.
Comment on attachment 8590476 [details] [diff] [review] fix sec-approval+ for trunk. Please create and nominate Aurora, Beta, and ESR31 patches.
Comment on attachment 8590476 [details] [diff] [review] fix Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: possible exploitable crashes in rare circumstances, but there are no known ways to trigger it ATM. [Describe test coverage new/current, TreeHerder]: green on Try, this code is well covered by tests [Risks and why]: low-risk [String/UUID change made/needed]: none
Created attachment 8591160 [details] [diff] [review] ESR31 patch [Approval Request Comment] see above
Comment on attachment 8590476 [details] [diff] [review] fix Should be in beta 4.
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3a999c888481 https://hg.mozilla.org/releases/mozilla-b2g34_v2_12/rev/3a999c888481 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/66b55766c8e4 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/66b55766c8e4 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4fff6bfb4963
Is there a need for manually testing this fix? If yes, could you please provide some detailed steps on how to do it?
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #21) > Is there a need for manually testing this fix? No.