Closed Bug 607494 Opened 9 years ago Closed 9 years ago

consider adding protection against constructing duplicate primary frames

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: dbaron, Assigned: tnikkel)

Details

(Whiteboard: [sg:want] may prevent critical bugs)

Attachments

(2 files)

It would have helped bug 607222 if we had protection against construction of duplicate frames for a content node.  It doesn't seem like this should be hard to do.  It's possible we even have it on mozilla-central already, given the fix window in bug 607222 comment 11, although I don't see how:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fa1e6f870ff1&tochange=f60b3bbfa8ce

(There's also an assertion in NeedFrameFor that should fire in this case, added in http://hg.mozilla.org/mozilla-central/rev/c410241f9d1c , though I don't see bug 607222 on trunk (even with html5 turned off).)
I don't believe we have such protection.

I guess the proposal here is to just bail out of Content(Range)Inserted if GetPrimaryFrame() is non-null?
Shouldn't be too hard, just need to be careful about <area>'s. Lazy frame construction makes this less likely to happen I think, and adds more asserts that catch it if it does, but not all frames are constructed lazily still.
blocking2.0: --- → ?
Whiteboard: [sg:nse preventive] may prevent critical bugs
Whiteboard: [sg:nse preventive] may prevent critical bugs → [sg:want] may prevent critical bugs
ReplicateFixedFrames complicates this...
...because I think we would want to make the primary frame check in AddFrameConstructionItems, so that we construct frames for the rest of the nodes in the ContentAppended case if one of them has a primary frame.
We have a flag in the frame constructor state that tells us when we're inside ReplicateFixedFrames (or inside replicating table headers/footers).
Ah, perfect.
Attached patch Add asserts.Splinter Review
This adds asserts to catch the problem. The next part actually stops trying to create the frames.
Attachment #488128 - Flags: review?(bzbarsky)
This actually stops creating the frames.
Attachment #488129 - Flags: review?(bzbarsky)
Although on second thought if we are appending/inserting more than one node and one of them has a primary frame when we try to put the new frames into the existing frame tree that one pre-existing frame is going to mess things up. Maybe we are better off just not creating frames for any of the new nodes?
Comment on attachment 488128 [details] [diff] [review]
Add asserts.

This looks fine.
Attachment #488128 - Flags: review?(bzbarsky) → review+
What sorts of "mess things up" are you worried about in comment 9?
Let's say we append (or insert) nodes 4 5 6 7 and 6 already has a primary frame. So we construct frames for 4 5 7 and insert them in order at whatever insertion point we have, the frame tree would then probably look like 4 5 7 6.
That seems better than crashing, and also possibly better than not showing the content.... but I see your point about this last.

Do we have known ways of getting into this situation?
Alternatively we could do a primary frame check at the start of ContentAppended and ContentRangeInserted and issue notifications for the ranges of nodes that don't have a primary frame. Lazy frame construction can be modified to ignore the NEEDS_FRAME bit when the node already has a primary frame.
That won't handle cases where some descendant of the node being appended/inserted already has a frame, right?
True. But if its parent node doesn't have a frame and the child node does then the frame is probably not linked into the frame tree, perhaps a less likely situation. Need some more thought on that case.
Comment on attachment 488129 [details] [diff] [review]
Don't create the frames

r=me
Attachment #488129 - Flags: review?(bzbarsky) → review+
Landed
http://hg.mozilla.org/mozilla-central/rev/a352c0afa631
http://hg.mozilla.org/mozilla-central/rev/c6a2526886f9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Any reason this needs to be hidden?
Probably not.

Have you tested that this also fixes bug 607222 if the patch that was landed to fix it is reverted?
It is not very straight forward to test if these patches fix bug 607222 (without the patch for bug 607222) because we need the patch for bug 563837 for the patches in this bug, which landed May 2010, but bug 607222 was fixed on mozilla-central in December 2009.
bug 607222 was fixed on m-c one month ago (and the patch in it should be relatively straightforward to back out, I'd think; though if you go that route you should make sure the crash happens following the backout).
I was going by the fix range in bug 607222 comment 11. Is there a testcase for bug 607222 that I can test on m-c that fails without the patches from bug 607222?
Oh, right.  Never mind then.
Assignee: nobody → tnikkel
Group: core-security
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.