599080 - Frames leak when failing to insert them in the frame tree

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[(no longer active)]

Over in bug 595337, I added a test to test_printpreview.xul to make sure that previewing a page doesn't crash the browser.  The test caused leaks on mozilla-central.  I tested the patch locally on Linux, and running test_printpreview.xul by itself is enough to cause the leak.  The patch and test in question are available in attachment 474884 [details] [diff] [review].

Now, all I had to do to fix the leak was to remove the timeout which was there after loading the content, and before calling printPreview.  Here's an interdiff between the original patch and a patch which fixes the issue: <https://bugzilla.mozilla.org/attachment.cgi?oldid=474884&action=interdiff&newid=477645&headers=1>.

This doesn't make a lot of sense, so I'm filing a new bug for investigating this issue.

Comment 1

[(no longer active)]

So, this doesn't really have anything specific to do with printing.  What's happening is that inside nsCSSFrameConstructor::ContentRangeInserted, we call nsFrameManager::InsertFrames (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7245), which ends up calling nsCanvasFrame::InsertFrames, which fails because aPrevFrame is non-null (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsCanvasFrame.cpp#184).  This calls the nsFrameManager::InsertFrames call to fail, in which case we'll leak the frames inside the nsFrameList object passed to it.

The fix is simple, we should just destroy the frames if that call fails.  I'll post a patch right away.

And this should block, because it's now a known memory leak case, and because it blocks bug 595337 which is a final blocker, as this leak occurs with the test case in that bug.

Comment 2

[(no longer active)]

Attachment: Patch (v1)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> which ends up calling nsCanvasFrame::InsertFrames

Sounds awfully like bug 482886....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 481093 [details] [diff] [review]
Patch (v1)

This looks like it will destroy any out-of-flows pointed to from inside frameItems but leave them in the frame constructor state to be inserted into the frame tree.  Probably to be followed by a crash pretty soon.

AppendFrames/InsertFrames really needs to not fail.  Why is it failing?

Comment 5

[(no longer active)]

(In reply to comment #3)
> > which ends up calling nsCanvasFrame::InsertFrames
> 
> Sounds awfully like bug 482886....

Oh yeah!  This seems like an exact case of that bug, except that we're leaking as well!

(In reply to comment #4)
> This looks like it will destroy any out-of-flows pointed to from inside
> frameItems but leave them in the frame constructor state to be inserted into
> the frame tree.  Probably to be followed by a crash pretty soon.

I'm not sure if I know what you're talking about, but perhaps that's something we want to fix in nsFrameList::DestroyFrames?  (More information would be pretty helpful.  FWIW, the try server seems pretty happy with me on this patch...)

> AppendFrames/InsertFrames really needs to not fail.  Why is it failing?

This assertion triggers <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsCanvasFrame.cpp#182> and then we'll fail here <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsCanvasFrame.cpp#184>.  But are we certain that AppendFrames/InsertFrames never fail in practice?  What if they do?  Are we OK with leaking all the frames, and anything else associated with them (content nodes and more)?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> but perhaps that's something we want to fix in nsFrameList::DestroyFrames?

We can't.  The framelist has no information about the frame constructor state holding on to the out-of-flows.... We used to have a buggy and very complicated function to tear down that state but we got rid of it when we switched to a "don't destroy the frames after creating them but before fully inserting them in the frame tree" setup.

More information... See the ProcessFrameInsertions calls in ~nsFrameConstructorState.  The way this works is that the in-flow frames (including placeholders) go in the frame list; their out-of-flows go into the lists in the frame constructor state.  When that's destroyed it puts the out of flows into the right places in the tree.  DestroyFrames will destroy the out-of-flows, but knows nothing about these random other pointers to them.

> FWIW, the try server seems pretty happy with me on this patch...

That mostly tells me we don't have very good test coverage on this codepath. :(

> But are we certain that AppendFrames/InsertFrames never fail in practice? 

No, but we're close to it....   As I said, I think we should fix them to never fail in practice.

> Are we OK with leaking all the frames, and anything else associated with them
> (content nodes and more)?

I'm more ok with that than with crashing.  :(

I think we should consider just allowing the insert/append there to proceed, just like we allow the bogus SetInitialChildList in bug 482886 to proceed....

Comment 7

[(no longer active)]

(In reply to comment #6)
> > but perhaps that's something we want to fix in nsFrameList::DestroyFrames?
> 
> We can't.  The framelist has no information about the frame constructor state
> holding on to the out-of-flows.... We used to have a buggy and very complicated
> function to tear down that state but we got rid of it when we switched to a
> "don't destroy the frames after creating them but before fully inserting them
> in the frame tree" setup.
> 
> More information... See the ProcessFrameInsertions calls in
> ~nsFrameConstructorState.  The way this works is that the in-flow frames
> (including placeholders) go in the frame list; their out-of-flows go into the
> lists in the frame constructor state.  When that's destroyed it puts the out of
> flows into the right places in the tree.  DestroyFrames will destroy the
> out-of-flows, but knows nothing about these random other pointers to them.

Hmm, I see... :(

> > FWIW, the try server seems pretty happy with me on this patch...
> 
> That mostly tells me we don't have very good test coverage on this codepath. :(

That's really sad.  Someone equipped with layout-fu should probably address that.  /me looks at roc who's on vacation...

> > But are we certain that AppendFrames/InsertFrames never fail in practice? 
> 
> No, but we're close to it....   As I said, I think we should fix them to never
> fail in practice.

OK.  Do you have bug numbers which are tracking to make sure that they never fail in practice?  It would be helpful to set this bug depend on those.  BTW, are we talking post-2.0?

> > Are we OK with leaking all the frames, and anything else associated with them
> > (content nodes and more)?
> 
> I'm more ok with that than with crashing.  :(
> 
> I think we should consider just allowing the insert/append there to proceed,
> just like we allow the bogus SetInitialChildList in bug 482886 to proceed....

Hmm, in that case, I guess I could land the fix to bug 595337 without the test, and post a patch to add the test here whenever this bug is fixed.  While I really don't like that, I don't like the wallpaper fixes that we've taking for the same underlying issue a bit more...

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Do you have bug numbers which are tracking to make sure that they never
> fail in practice? 

We don't have them yet.  Should file.  And yes, post-2.0.

Note that it seems like this bug is basically a leak any time a particular kind of document is printed; if we can wallpaper it easily for 2.0, we should.

Comment 9

[(no longer active)]

(In reply to comment #8)
> Note that it seems like this bug is basically a leak any time a particular kind
> of document is printed; if we can wallpaper it easily for 2.0, we should.

So is my patch good enough as a wallpaper fix?

Comment 10

[Boris Zbarsky [:bzbarsky]]

No, since it converts a leak into a (possibly exploitable, depending on how much you trust frame poisoning) crash...

Comment 11

[(no longer active)]

(In reply to comment #6)
> More information... See the ProcessFrameInsertions calls in
> ~nsFrameConstructorState.  The way this works is that the in-flow frames
> (including placeholders) go in the frame list; their out-of-flows go into the
> lists in the frame constructor state.  When that's destroyed it puts the out of
> flows into the right places in the tree.  DestroyFrames will destroy the
> out-of-flows, but knows nothing about these random other pointers to them.

So, what if we clear those four out-of-flow lists stored in the state object?  Would that be good enough?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'm not sure.  Again, we used to have some pretty complicated code here, but maybe Elika's frame teardown changes made it better...

What's wrong with my suggestion from comment 6 last paragraph?

Comment 13

[(no longer active)]

(In reply to comment #12)
> I'm not sure.  Again, we used to have some pretty complicated code here, but
> maybe Elika's frame teardown changes made it better...

Elika, do you think that's the case?

> What's wrong with my suggestion from comment 6 last paragraph?

Nothing in particular, I was not sure if that's safe, but I'll give that a shot.

Comment 14

[(no longer active)]

Attachment: Patch (v2)

OK, this indeed makes the leak go away.  I'm running the entire test suite locally to make sure that it's at least safe enough as far as our test coverage tells us.

Comment 15

[(no longer active)]

OK, every single one of our tests passed successfully with this patch (sans known intermittent oranges).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 481322 [details] [diff] [review]
Patch (v2)

But AppendFrames will just return an error if !mFrames.IsEmpty().... so this still returns an error.

I'd rather we copied AppendFrames, removed the mFrames.IsEmpty() checks from both, and made this function call InsertFrames on mFrames as needed.

Comment 17

[(no longer active)]

(In reply to comment #16)
> Comment on attachment 481322 [details] [diff] [review]
> Patch (v2)
> 
> But AppendFrames will just return an error if !mFrames.IsEmpty().... so this
> still returns an error.

mFrames is empty in this case...  The problem is that we expect there not to be a previous sibling frame, but there is one.

> I'd rather we copied AppendFrames, removed the mFrames.IsEmpty() checks from
> both, and made this function call InsertFrames on mFrames as needed.

You mean something like this:

NS_IMETHODIMP
nsCanvasFrame::InsertFrames(...) {
  // check for absolute list

  return AppendFramesNoCheck(...);
}

NS_IMETHODIMP
nsCanvasFrame::AppendFramesNoCheck(...) {
  // exact same thing as AppendFrames, except for mFrame.IsEmpty() checks
}

?

I can give it a shot tomorrow as well.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> but there is one.

So aPrevSibling is not in mFrames?

Comment 19

[fantasai]

> The way this works is that the in-flow frames (including placeholders) go in
> the frame list; their out-of-flows go into the lists in the frame constructor
> state.  When that's destroyed it puts the out of flows into the right places
> in the tree.

So, I haven't looked at the code yet, but if I'm understanding the comments here correctly, and the goal here is to destroy the frame on an insertion fail, the best way to fix this would be the following:

First, make sure that if the oof in nsPlaceholderFrame::DestroyFrom() has a null parent, it hits this else clause instead of attempting to remove the frame:
  // else oof will be destroyed by its parent
(and update the comment accordingly)

Second, when you destroy the oof frame list in nsCSSFrameConstructor, check that the OOF has a placeholder. If it doesn't, destroy it instead of placing it in the tree.

Third, if an insert fails, Destroy() the frame.

Let me know if that doesn't make sense. :)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> check that the OOF has a placeholder.

That can make some cases that are currently O(1) be O(N) instead, no?  (The common case of initial construction of the out-of-flow list of some parent.)

Comment 21

[(no longer active)]

(In reply to comment #17)
> (In reply to comment #16)
> > Comment on attachment 481322 [details] [diff] [review]
> > Patch (v2)
> > 
> > But AppendFrames will just return an error if !mFrames.IsEmpty().... so this
> > still returns an error.
> 
> mFrames is empty in this case...  The problem is that we expect there not to be
> a previous sibling frame, but there is one.

Everything I said here can be totally ignored, I was wrong.

Comment 22

[fantasai]

> That can make some cases that are currently O(1) be O(N) instead, no?

You mean cases where the whole list is inserted into a single parent? Don't you need to go through that list to Reparent all its members anyway?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Usually no; out-of-flows are created with the right parent to start with.

And the whole list is always inserted into a single parent; the only question is whether it's all inserted at the same _spot_.

Comment 24

[fantasai]

Then if you want to avoid an O(N) search, you have to find some other way of keeping track of which frames in that list need to be deleted. I don't think creating a bogus frame tree structure in order to avoid this leak is a good idea.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Well, hold on.  The only reason the frame tree structure is "bogus" is that the frame constructor is violating invariants that CanvasFrame has due to the way fixed-pos replication is implemented.  You did read bug 482886, right?

So yes, we could destroy those out of flows.  That means breaking fixed-pos printing, no?  Or am I missing something?

Comment 26

[fantasai]

I see two problems here. One is bug 482886, which is that replication breaks CanvasFrame invariants. The other is this one, which is that CSSFrameConstructor causes leaks when frame insertion fails. If both bugs are fixed, then there's no problem, right?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> If both bugs are fixed, then there's no problem, right?

Yes, but my point is that I don't see a good reason to have any situations where "frame insertion fails"...

Comment 28

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

We should be able to detect that the new frames are going to be inserted into an nsCanvasFrame and not even create them in the first place.

The frame duplication stuff can't handle IFRAMEs, because we can't create two separate presentations at different locations. So we should probably add some check to simply not even try to duplicate nsSubdocumentFrames.

Comment 29

[(no longer active)]

Attachment: Patch (v1)

This implements comment 28.

Comment 30

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

What if the <iframe> is a child of the position:fixed content? Won't we have the same problem?

Comment 31

[(no longer active)]

(In reply to comment #30)
> What if the <iframe> is a child of the position:fixed content? Won't we have
> the same problem?

Hmm, you're right.  Should I add a new flag to nsFrameConstructorState to represent whether we're creating replicated frames, and check for it in nsCSSFrameConstructor::ConstructFrameFromItemInternal?

Comment 32

[Boris Zbarsky [:bzbarsky]]

We already have such a flag.  It's called mSetPrimaryFrame.  Might need renaming.  ;)

Comment 33

[(no longer active)]

(In reply to comment #32)
> We already have such a flag.  It's called mSetPrimaryFrame.  Might need
> renaming.  ;)

Huh, yeah!  The name did not suggest that is the only use case!  So should I assume that the solution I proposed in comment 31 is an acceptable one?

Comment 34

[Boris Zbarsky [:bzbarsky]]

It sounds ok to me... however it doesn't really solve this bug, right?

Comment 35

[(no longer active)]

(In reply to comment #34)
> It sounds ok to me... however it doesn't really solve this bug, right?

Well, it doesn't solve the general problem, but it masks the problem which is preventing me from landing bug 595337 (which was the main reason that I filed this bug).  Maybe I should re-summarize this and file another bug for the general case?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Why does it mask the problem?  Do we not still leak frames?  Or is it that the test has only a subdocument frame on the fixed list?

Comment 37

[(no longer active)]

(In reply to comment #36)
> Why does it mask the problem?  Do we not still leak frames?

We refrain from creating frames for which the insertion will fail later on.

> Or is it that the
> test has only a subdocument frame on the fixed list?

This is the case with the testcase, yes.

Comment 38

[Boris Zbarsky [:bzbarsky]]

OK, but printing any page with fixed-pos stuff on it will still leak, right?  I'd rather we just fixed that (possibly in addition to the other thing).

Comment 39

[(no longer active)]

(In reply to comment #38)
> OK, but printing any page with fixed-pos stuff on it will still leak, right? 

No, unless I'm missing something...

Comment 40

[Boris Zbarsky [:bzbarsky]]

Do we not end up leaking every time we have to replicate fixed-pos things?  If not, why not?

Comment 41

[(no longer active)]

This type of leak only happens when we try to insert frames into a canvas frame wehre aPrevFrame is non-null (see comment 1).  And the reason of the leak is that trying to insert frames in that way causes the insertion to fail, and we don't destroy the frames which failed to get inserted, hence the leak.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Oh, right.  What causes aPrevFrame to be non-null in this case?

Comment 43

[(no longer active)]

Here's the stack that gets us to the failing call:

 	xul.dll!nsCanvasFrame::InsertFrames(nsIAtom * aListName, nsIFrame * aPrevFrame, nsFrameList & aFrameList)  Line 177	C++
 	xul.dll!nsFrameManager::InsertFrames(nsIFrame * aParentFrame, nsIAtom * aListName, nsIFrame * aPrevFrame, nsFrameList & aFrameList)  Line 488	C++
>	xul.dll!nsCSSFrameConstructor::ContentRangeInserted(nsIContent * aContainer, nsIContent * aStartChild, nsIContent * aEndChild, nsILayoutHistoryState * aFrameState, int aAllowLazyConstruction)  Line 7273	C++
 	xul.dll!nsCSSFrameConstructor::ContentInserted(nsIContent * aContainer, nsIContent * aChild, nsILayoutHistoryState * aFrameState, int aAllowLazyConstruction)  Line 6771	C++
 	xul.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent, int aAsyncInsert)  Line 9076 + 0x23 bytes	C++
 	xul.dll!nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList & aChangeList)  Line 7941	C++
 	xul.dll!nsCSSFrameConstructor::RestyleElement(mozilla::dom::Element * aElement, nsIFrame * aPrimaryFrame, nsChangeHint aMinHint, mozilla::css::RestyleTracker & aRestyleTracker, int aRestyleDescendants)  Line 8029	C++
 	xul.dll!mozilla::css::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aChangeHint)  Line 157	C++
 	xul.dll!mozilla::css::RestyleTracker::ProcessRestyles()  Line 241	C++
 	xul.dll!nsCSSFrameConstructor::ProcessPendingRestyles()  Line 11598	C++
 	xul.dll!PresShell::FlushPendingNotifications(mozFlushType aType)  Line 4875	C++
 	xul.dll!PresShell::HandlePostedReflowCallbacks(int aInterruptible)  Line 4768	C++
 	xul.dll!PresShell::DidDoReflow(int aInterruptible)  Line 7621	C++
 	xul.dll!PresShell::ProcessReflowCommands(int aInterruptible)  Line 7912	C++
 	xul.dll!PresShell::FlushPendingNotifications(mozFlushType aType)  Line 4905 + 0x12 bytes	C++
 	xul.dll!nsPrintEngine::ReflowPrintObject(nsPrintObject * aPO)  Line 2028	C++
 	xul.dll!nsPrintEngine::ReflowDocList(nsPrintObject * aPO, int aSetPixelScale)  Line 1864 + 0xc bytes	C++
 	xul.dll!nsPrintEngine::SetupToPrintContent()  Line 1674 + 0x16 bytes	C++
 	xul.dll!nsPrintEngine::DocumentReadyForPrinting()  Line 1506 + 0x8 bytes	C++
 	xul.dll!nsPrintEngine::FinishPrintPreview()  Line 3273 + 0x8 bytes	C++
 	xul.dll!nsPrintEngine::DoCommonPrint(int aIsPrintPreview, nsIPrintSettings * aPrintSettings, nsIWebProgressListener * aWebProgressListener, nsIDOMDocument * aDoc)  Line 716 + 0xb bytes	C++
 	xul.dll!nsPrintEngine::CommonPrint(int aIsPrintPreview, nsIPrintSettings * aPrintSettings, nsIWebProgressListener * aWebProgressListener, nsIDOMDocument * aDoc)  Line 445 + 0x18 bytes	C++
 	xul.dll!nsPrintEngine::PrintPreview(nsIPrintSettings * aPrintSettings, nsIDOMWindow * aChildDOMWin, nsIWebProgressListener * aWebProgressListener)  Line 790 + 0x1b bytes	C++
 	xul.dll!DocumentViewerImpl::PrintPreview(nsIPrintSettings * aPrintSettings, nsIDOMWindow * aChildDOMWin, nsIWebProgressListener * aWebProgressListener)  Line 3730 + 0x28 bytes	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params)  Line 103	C++
 	xul.dll!CallMethodHelper::Invoke()  Line 3081 + 0x1c bytes	C++
 	xul.dll!CallMethodHelper::Call()  Line 2348 + 0x8 bytes	C++
 	xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode)  Line 2312 + 0x16 bytes	C++
 	xul.dll!XPC_WN_CallMethod(JSContext * cx, unsigned int argc, jsval_layout * vp)  Line 1752 + 0xe bytes	C++
 	mozjs.dll!js::Interpret(JSContext * cx, JSStackFrame * entryFrame, unsigned int inlineCallCount, unsigned int interpFlags)  Line 4625 + 0x1d bytes	C++
 	mozjs.dll!js::RunScript(JSContext * cx, JSScript * script, JSFunction * fun, JSObject & scopeChain)  Line 638 + 0x16 bytes	C++
 	mozjs.dll!js::Invoke(JSContext * cx, const js::CallArgs & argsRef, unsigned long flags)  Line 746 + 0x1a bytes	C++
 	mozjs.dll!js::ExternalInvoke(JSContext * cx, const js::Value & thisv, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval)  Line 776 + 0xf bytes	C++
 	mozjs.dll!js::ExternalInvoke(JSContext * cx, JSObject * obj, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval)  Line 937 + 0x2a bytes	C++
 	mozjs.dll!JS_CallFunctionValue(JSContext * cx, JSObject * obj, jsval_layout fval, unsigned int argc, jsval_layout * argv, jsval_layout * rval)  Line 4836 + 0x38 bytes	C++
 	xul.dll!nsJSContext::CallEventHandler(nsISupports * aTarget, void * aScope, void * aHandler, nsIArray * aargv, nsIVariant * * arv)  Line 2140 + 0x2e bytes	C++
 	xul.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout)  Line 8829 + 0xb1 bytes	C++
 	xul.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer, void * aClosure)  Line 9177	C++
 	xul.dll!nsTimerImpl::Fire()  Line 425 + 0xe bytes	C++
 	xul.dll!nsTimerEvent::Run()  Line 519	C++
 	xul.dll!nsThread::ProcessNextEvent(int mayWait, int * result)  Line 547 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, int mayWait)  Line 250 + 0x16 bytes	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate)  Line 110 + 0xe bytes	C++
 	xul.dll!MessageLoop::RunInternal()  Line 220	C++
 	xul.dll!MessageLoop::RunHandler()  Line 203	C++
 	xul.dll!MessageLoop::Run()  Line 177	C++
 	xul.dll!nsBaseAppShell::Run()  Line 186	C++
 	xul.dll!nsAppShell::Run()  Line 243 + 0x9 bytes	C++
 	xul.dll!nsAppStartup::Run()  Line 191 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData)  Line 3670 + 0x25 bytes	C++
 	firefox.exe!NS_internal_main(int argc, char * * argv)  Line 158 + 0x12 bytes	C++
 	firefox.exe!wmain(int argc, wchar_t * * argv)  Line 129 + 0xd bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
 	firefox.exe!wmainCRTStartup()  Line 403	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes	

prevSibling in nsCSSFrameConstructor::ContentRangeInserted is set to the placeholder frame, which is also the child of the same canvas frame.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Ah, so the key is the restyle processing, which gives us what looks like a dynamic append?

Comment 45

[(no longer active)]

(In reply to comment #44)
> Ah, so the key is the restyle processing, which gives us what looks like a
> dynamic append?

It seems so, yes.

Comment 46

[(no longer active)]

Attachment: Part 1: Rename mSetPrimaryFrame

Comment 47

[(no longer active)]

Attachment: Part 2: don't create subdocument frames for replicated content

roc, does this address comment 30 correctly?

Comment 48

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

Why do you need the second hunk?

Comment 49

[(no longer active)]

Attachment: Part 2: don't create subdocument frames for replicated content

Meant to remove it...

Comment 50

[(no longer active)]

Comment on attachment 484070 [details] [diff] [review]
Part 1: Rename mSetPrimaryFrame

I had forgot to ask for review on this patch!

Comment 51

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8c2e262548fd
http://hg.mozilla.org/mozilla-central/rev/699b6226f2a2