607494 - consider adding protection against constructing duplicate primary frames

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).)

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Timothy Nikkel (:tnikkel)]

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.

Comment 3

[Timothy Nikkel (:tnikkel)]

ReplicateFixedFrames complicates this...

Comment 4

[Timothy Nikkel (:tnikkel)]

...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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

We have a flag in the frame constructor state that tells us when we're inside ReplicateFixedFrames (or inside replicating table headers/footers).

Comment 6

[Timothy Nikkel (:tnikkel)]

Ah, perfect.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Add asserts.

This adds asserts to catch the problem. The next part actually stops trying to create the frames.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Don't create the frames

This actually stops creating the frames.

Comment 9

[Timothy Nikkel (:tnikkel)]

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 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488128 [details] [diff] [review]
Add asserts.

This looks fine.

Comment 11

[Boris Zbarsky [:bzbarsky]]

What sorts of "mess things up" are you worried about in comment 9?

Comment 12

[Timothy Nikkel (:tnikkel)]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Timothy Nikkel (:tnikkel)]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

That won't handle cases where some descendant of the node being appended/inserted already has a frame, right?

Comment 16

[Timothy Nikkel (:tnikkel)]

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 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488129 [details] [diff] [review]
Don't create the frames

r=me

Comment 18

[Timothy Nikkel (:tnikkel)]

Landed
http://hg.mozilla.org/mozilla-central/rev/a352c0afa631
http://hg.mozilla.org/mozilla-central/rev/c6a2526886f9

Comment 19

[Timothy Nikkel (:tnikkel)]

Any reason this needs to be hidden?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Probably not.

Have you tested that this also fixes bug 607222 if the patch that was landed to fix it is reverted?

Comment 21

[Timothy Nikkel (:tnikkel)]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 23

[Timothy Nikkel (:tnikkel)]

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?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, right.  Never mind then.