578604 - Crash [@ nsTextControlFrame::SetInitialChildList(nsIAtom*, nsFrameList&) ] when print preview

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100713 Firefox/4.0b2pre ID:20100713040624
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100713 Firefox/4.0b2pre ID:20100713040624

On New Profile,
When entering print preview, the browser crashes immediately.

Crash reports:
bp-b025ca64-5b66-42b2-9d89-738a32100713
bp-d9920aa8-c63c-4b04-b71e-24d572100713
bp-922b9225-179e-437f-9f18-375882100713
bp-5fc58e96-6bf3-4d0e-8a55-682a92100713
bp-e63afd02-875a-4329-a61b-f0dcf2100713
bp-03b0a48e-ca8a-48f7-984f-bd1392100713
bp-bf2df7fc-042b-45e0-891a-316ef2100713
bp-aeabcf6a-17aa-4a2d-8d77-ed07f2100713

Bug 541506 is same Crash ID, but different condition.


Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open URL ( http://www.whatwg.org/specs/web-apps/current-work/ )
3. *Wait* completion of page loading
4. Print preview ( App button > Right arrow of print)

Actual Results:
 Browser crashes with crash report

Expected Results:
 Not crash

Regression window:

Works:
http://hg.mozilla.org/mozilla-central/rev/21f0727c27a6
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100605 Minefield/3.7a5pre ID:20100605111313

Fails:
http://hg.mozilla.org/mozilla-central/rev/2875f9a693ae
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100605 Minefield/3.7a5pre ID:20100605135617

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21f0727c27a6&tochange=2875f9a693ae

Comment 1

[Boris Zbarsky [:bzbarsky]]

My money is on bug 534785 as the cause.  Seems eerily similar to bug 570566.  Ehsan, can you take a look?

Comment 2

[Alice0775 White]

Attachment: minimum test case

Comment 3

[(no longer active)]

So, what I'm seeing here is that nsCSSFrameConstructor::ReplicateFixedFrames constructs a frame for the input node *without* destroying the existing frame.  This causes the nsTextEditorState::BindToFrame call to fail, which makes nsTextControlFrame::CreateAnonymousContent to return without actually constructing the anonymous content.  Therefore no frame gets constructed for them, and nsTextControlFrame::SetInitialChildList blows away because it assumes there to be at least one frame under the text control frame.

Throughout my work in detangling the text control frames from editors, I made the assumption that there can only be one live frame for any content node.  If this assumption doesn't hold (reading nsCSSFrameConstructor::ReplicateFixedFrames makes me believe so), then pretty much all of that work needs to be redone, I'm afraid.  :(

Does anyone know if this frame-content polygamy is allowed/expected?

Comment 4

[Boris Zbarsky [:bzbarsky]]

For the specific case of text control frames, I believe the ick that is ReplicateFixedFrames is the only way to get into this situation....

Comment 5

[(no longer active)]

(In reply to comment #4)
> For the specific case of text control frames, I believe the ick that is
> ReplicateFixedFrames is the only way to get into this situation....

Hmm, so, can I just destroy the old frame before constructing a new one (for text control frames, that is)?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, that would make it print wrong, no?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Specifically, <input style="position: fixed"> is supposed to print with the input appearing on each page of the printout.

Comment 8

[(no longer active)]

Hmm...  In that case I can't see how to fix this except by backing out bug 534785, which I really don't want to do...  :-(

I also assume that it's not possible to clone the content nodes, or something, right?

Comment 9

[Boris Zbarsky [:bzbarsky]]

You mean the <input> node?  Not easily...

What's the issue with making the text input just paint the right thing? It doesn't actually need an editor when printing, right?

Comment 10

[(no longer active)]

(In reply to comment #9)
> You mean the <input> node?  Not easily...
> 
> What's the issue with making the text input just paint the right thing? It
> doesn't actually need an editor when printing, right?

It doesn't.  But we store the anonymous div for input controls on the content node, so we can't really have two of them per input node.  Also, if the input control has spellcheck=true, then we'd have to create an editor for it as well...

Comment 11

[Boris Zbarsky [:bzbarsky]]

But we know we're not the "real" frame.  In that situation, can we not just skip all this stuff and hack painting to do the right thing?  That should be all we need for printing, right?

Comment 12

[(no longer active)]

How do we know that we're not a real frame?  As far as I can see, all we see is a frame being constructed, we don't know what it's going to be used for, right?

Also, in case of spellcheck, getting the correct painting is not very easy to hack either.

Comment 13

[Boris Zbarsky [:bzbarsky]]

We can check whether the content already has a non-null primary frame.

Comment 14

[Boris Zbarsky [:bzbarsky]]

And I'm not sure why we can't just paint the primary frame of the content when asked to paint...

Comment 15

[(no longer active)]

(In reply to comment #14)
> And I'm not sure why we can't just paint the primary frame of the content when
> asked to paint...

I'm not sure how we would go about doing that...

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There is another case like this --- table header/footer replication.

I'm comfortable with making both of these paint-time-only effects. That would mean we can't vary the widget of position:fixed content that appears on pages of different widths, ditto for repeated table headers/footers. Does anyone think that's not OK?

If so, one approach would be to create a special kind of frame that paints itself by creating a display list for the primary frame of the content and painting that display list. There would be some tricky wrapping required so that all the items in the display list can be exposed individually, so that replicated content can be correctly interleaved in z-order with non-replicated content.

Comment 17

[(no longer active)]

(In reply to comment #16)
> If so, one approach would be to create a special kind of frame that paints
> itself by creating a display list for the primary frame of the content and
> painting that display list. There would be some tricky wrapping required so
> that all the items in the display list can be exposed individually, so that
> replicated content can be correctly interleaved in z-order with non-replicated
> content.

I have no idea off hand on how to do that.  Is there anyone else better suited to the job than me?  I wouldn't mind taking a stab at this, but it would be kind of a big project for me, and I have other blockers on my plate, and I hate to take a blocker for which I don't have any idea of an ETA.

Also, if we have such a frame class, could we use that to replicate any kind of frame, not just the text control frame?  That would be kind of useful, because in that case one can just assume that every content node has at most one live frame at any given time, which could be a useful assumption to make elsewhere as well.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #17)
> Also, if we have such a frame class, could we use that to replicate any kind of
> frame, not just the text control frame?  That would be kind of useful, because
> in that case one can just assume that every content node has at most one live
> frame at any given time, which could be a useful assumption to make elsewhere
> as well.

Exactly.

IMHO you should wallpaper over this crash bug for now by making nsTextControlFrame::SetInitialChildList handle the case where there's no anonymous content. We won't display the contents of the input text in fixed-pos elements when printing, that's all.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> because in that case one can just assume that every content node has at most
> one live frame

For the general case, you cannot in fact assume that.  It would be something that could be assumed for replaced elements, though.

roc, good catch on table headers/footers....  I agree that given those just wallpapering this is the way to go.

Comment 20

[(no longer active)]

I filed bug 584564 to implement roc's idea in comment 16 as the correct fix.

Comment 21

[(no longer active)]

Attachment: Wallpaper

The crash test asserts once, which is expected, because we're not doing the right thing (we're trying to bind a second frame to the content node when the first one has not yet been unbound), and I've marked that assertion to be fixed by bug 584564.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 463000 [details] [diff] [review]
Wallpaper

+  if (first)
+    first->AddStateBits(NS_FRAME_REFLOW_ROOT);

{}

Comment 23

[(no longer active)]

Landed with the nit addressed.

http://hg.mozilla.org/mozilla-central/rev/def49cb243f0