Closed Bug 508473 Opened 15 years ago Closed 14 years ago

Clean up and reorganize frame destruction

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- wanted

People

(Reporter: fantasai.bugs, Assigned: fantasai.bugs)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(7 files, 5 obsolete files)

2.48 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
101.94 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
50.50 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
32.04 KB, patch
Details | Diff | Splinter Review
15.26 KB, patch
Details | Diff | Splinter Review
98.47 KB, patch
Details | Diff | Splinter Review
24.82 KB, application/zip
Details
We have a lot of frame deletion functions. It's pretty confusing, so I'd like to go through them and see if we can merge them into a better set of APIs. Also I'd like to address bug 411835 comment 18. I think we should be deleting continuations before next-siblings: having bits and piece of a frame scattered in the tree is not very safe.
Blocks: 508665
nsIFrame::RemovedAsPrimary does nothing except for nsTextControlFrame, where what it does is done by Destroy() soon after anyway.
See also bug 390762 comment 12 wrt RemoveFloat/DoRemoveFrame/DoRemoveOutOfFlow etc.
This removes DeletingFrameSubtree and shifts out its two functions into other methods:
  1. Unmapping content is now handled by nsFrameManager::NotifyDestroyingFrame()
     (which is called on all frames by nsFrame::Destroy()
  2. Destroying out-of-flows is now handled by nsPlaceholderFrame::Destroy()
     (This isn't optimized yet; it currently triggers Invalidate calls for all
     out-of-flow frames, even when not necessary.)
Blocks: 513877
> where what it does is done by Destroy() soon after anyway.

What happens in between? The whole point here, iirc, was that we needed to PreDestroy() early enough.  Did you check the blame and make sure we're not regressing anything?
RemovedAsPrimary was added in bug 132334. The testcase there does not crash, at least not with both patches here applied. I can rebuild and test with just Part I if you want.

What used to happen in between was DoDeletingSubtree completed its recurse. And in the case where the nsTextControlFrame was a descendent of nsListBoxBodyFrame, mLayoutManager->ChildrenRemoved() was called.
I guess the key question is whether after your patch we guarantee that we can access the primary frame of the editor's div via GetPrimaryFrameFor during PreDestroy (or if not, whether we can guarantee that nothing in PreDestroy needs any frames, which would be even better).
Attachment #396038 - Attachment is obsolete: true
Attachment #402293 - Flags: review?(bzbarsky)
Attachment #393332 - Flags: review?(bzbarsky)
This deals with the duplicate invalidation introduced by the previous patch for out-of-flow frames.
Attachment #402294 - Flags: review?(bzbarsky)
For the PreDestroy question... I'm not seeing where in PreDestroy we access the anonymous div or its frame. Maybe I'm missing some connection between it and the stuff that does go on in PreDestroy.

The Linux and OS X unittest boxes passed on tryserver, if that helps any. (Windows timed out.)
PreDestroy calls GetValue which calls OutputToString on the editor.  That will certainly touch the div itself.

I _think_ we're ok on the frame part, but I'm hoping you will in fact trace the codepaths from PreDestroy carefully before landing this.
What would I be looking for, any access to frames?
Yes.  (Sorry for lag; the bugmail's been coming in in weird orders.)
Comment on attachment 393332 [details] [diff] [review]
Patch Part I: Remove nsIFrame::RemovedAsPrimary

r=bzbarsky
Attachment #393332 - Flags: review?(bzbarsky) → review+
OK.  So I've been staring at part II for a while now, and I wanted to make sure I understand the new setup...

If I read correctly, it's always safe to destroy the placeholder before destroying it's out-of-flow.  Doing so will remove the out-of-flow from the frame tree.

If the out-of-flow is destroyed first, it will unregister the placeholder and null out the placeholder's mOutOfFlowFrame (in nsFrame:Destroy); after that we're sort of on hook until the placeholder is destroyed.

Sound about right?
So given the above, what happens in a situation where frame A has the placeholder for frame B as a descendant and frame B as its next sibling?  That's the situation you land in with an abs pos table containing an abs pos descendant, for example.  Looks to me like nsFrameList::DestroyFrames will get called, we will call Destroy on the table, which will destroy the placeholder and remove/destroy the table's next sibling, but DestroyFrames is already holding a ref to that next sibling, right?  Why do we not crash in that situation?
Oh, nevermind.  You changed DestroyFrames to not work like that.  OK, good!
Comment on attachment 402293 [details] [diff] [review]
Patch Part II: Remove DeletingFrameSubtree

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+nsCSSFrameConstructor::GetChildListNameFor(nsIFrame* aChildFrame)
>+    //XXXfr find a test that fails here

I can't make sense of this comment....

>+    if (childType == nsGkAtoms::menuPopupFrame) {
>+      listName = nsGkAtoms::popupList;

That doesn't seem quite right...  If a <menu> has multiple <popup> kids, only the first one eds up on popupList.

>+    } else {
>+      listName = nsnull;

This is wrong for captions.  Then again, for captions we need to use a different parent too, right?  Should we just be asserting that captions never get passed in here?

Should this method just move to nsLayoutUtils, if it's generally useful?  There's nothing frame constructor specific about it, right?

> nsCSSFrameConstructor::ContentRemoved(nsIContent* aContainer,
>+      if (childFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW) {
>+        childFrame = frameManager->GetPlaceholderFrameFor(childFrame);
>+        parentFrame = childFrame->GetParent();
>+        NS_ASSERTION(childFrame, "Missing placeholder frame for out of flow.");

That assert will never fire (since if !childFrame you'll crash on the ine before).  Move the assert up a line if you want to keep it?

>+      rv = frameManager->RemoveFrame(parentFrame, GetChildListNameFor(childFrame),

Can the child list name for a placeholder ever be non-null?  If not, is it worth putting the name in a local and only doing GetChildListNameFor() in the else case?

>+++ b/layout/base/nsCSSFrameConstructor.h
>+  static nsIAtom* GetChildListNameFor(nsIFrame* aChildFrame);

Could use documenting.

>+++ b/layout/base/nsFrameManager.cpp
>@@ -725,40 +725,40 @@ nsFrameManager::RemoveFrame(nsIFrame*   
>+  NS_ASSERTION(!aOldFrame->GetPrevContinuation() ||
>+               // exception for nsCSSFrameConstructor::RemoveFloatingFirstLetterFrames
>+               aOldFrame->GetType() == nsGkAtoms::textFrame,

Can we assert something about the parent in the textFrame case?  If so, please do.

>+  NS_ENSURE_SUCCESS(rv, rv);
> }

This returns a random value if rv is success.  At least with gcc.  I believe msvc will refuse to compile this.  Please either go back to |return rv| or put |return NS_OK| after the above NS_ENSURE_SUCCESS, if you were trying to make sure you got the warnings.

> nsFrameManager::NotifyDestroyingFrame(nsIFrame* aFrame)
>-      NS_NOTREACHED("frame was not removed from primary frame map before "
>-                    "destruction or was readded to map after being removed");

So that's the main upshot of the changes to the primary frame stuff here, right?  I guess this is fine; I'm hoping to nuke the primary frame map anyway.

>+  //XXXfr only need to do this for first continuations, but will need to
>+  // delete continuation chains backwards for an if-test here to work
>+  if (aFrame->GetContent()) {
>+    RemoveAsPrimaryFrame(aFrame->GetContent(), aFrame);
>+    ClearAllUndisplayedContentIn(aFrame->GetContent());

This looks wrong.  In particular, any time any frame whose mContent is a given content node is destroyed, we'll ClearAllUndisplayedContentIn() on that content.  In particular, if I have a <span> that line-wraps and then I make the parent block wider so it doesn't anymore we'll destroy the continuation and clear the undisplayed content under the span...  It should be possible to write a reftest that fails as a result.

Can we detect _that_ case here (not destroying the whole continuation chain, just part of it)?  Are there other situations where some but not all frames for a content node will be destroyed?

If nodes knew their primary frame we could easily condition the ClearAllUndisplayedContentIn() on aFrame being aFrame->GetContent()->GetPrimaryFrame(), I think.  But we haven't landed that yet.  :(

>+++ b/layout/generic/nsFrameList.cpp
> nsFrameList::DestroyFrames()

If we wanted to make RemoveFirstChild() return the old first child (null if there wasn't one), this could become:

  while (nsIFrame* frame = RemoveFirstChild()) {
    frame->Destroy();
  }

Either way, though.  I just like less sibling-chain munging; it's error-prone.  ;)

>+++ b/layout/generic/nsIFrame.h
>+   * Destroy() for each child). If this frame is a first-continuation, this
>+   * also unmaps the content associated with this frame. If it's a placeholder,

It's not really clear what unmaps means in this context.  Is that talking about the primary frame map?

>+++ b/layout/generic/nsPlaceholderFrame.cpp
> nsPlaceholderFrame::Destroy()
>+    if (shell && oof->GetParent()) {

So why exactly would |shell| be null here?  roc added that code; dbaron reviewed.  Check with them?  I'd think it should never be null in this code.

Also, what is the oof->GetParent() check trying to check, exactly?

>+      for (; oof; oof = oof->GetNextContinuation()) {
>+        NS_ASSERTION(!oof->GetParent(),
>+                     "Destroying half-inserted frame, this is really bad.");
>+        oof->Destroy();

What if |oof| is a newly-created frame (still on one of the frame construction state's nsFrameItems lists)?  It'll already have a parent (because Init() has been called on it), but won't be in the parent's child list....  On the other hand, the only ways to get here are via the DoCleanupFrameReferences call in AddChild (which doesn't destroy out of flows pointed to by placeholder descendants of aNewFrame) and via the CleanupFrameReferences call after FlushAccumulatedBlock (which looks to me like it never destroyed the out of flows for any kids of stuff in childItems).

Can we just assume this case won't happen, basically?  I believe those calls are both effectively OOM checks and we're going to remove those from the frame constructor as soon as Chris lands anything resembling infallible malloc...

In other words, can we just assume oof->GetParent() here (and assert accordingly)?  If not, why not?

>+++ b/layout/tables/nsTableFrame.cpp
> nsTableFrame::RemoveFrame(nsIAtom*        aListName,
>-    NS_ASSERTION(!aListName || aListName == nsGkAtoms::colGroupList,
>-                 "unexpected child list");
>+  if (aListName == nsGkAtoms::colGroupList) {

Can we assert that aListName == nsGkAtoms::colGroupList if and only if the frame is NS_STYLE_DISPLAY_TABLE_COLUMN_GROUP?  You assert that not the former implies not the latter, but not the converse.

>+++ b/layout/xul/base/src/nsPopupSetFrame.cpp
> nsPopupSetFrame::Destroy()
>+#ifdef DEBUG
>+      // nsFrame::Destroy asserts removal from the frame list
>+      mPopupList->mPopupFrame->SetNextSibling(nsnull);
>+#endif
>       mPopupList->mPopupFrame->Destroy();

Why is that #ifdef DEBUG here but not elsewhere?  I'd prefer we just did it unconditionally...

r- due to the undisplayed content issue, though the menupopup thing also needs sorting out, I think.
Attachment #402293 - Flags: review?(bzbarsky) → review-
> If I read correctly, it's always safe to destroy the placeholder before
> destroying it's out-of-flow.  Doing so will remove the out-of-flow from the
> frame tree.

Without Part III, yes. After Part III it will only remove the OOF if it detects that the OOF won't be deleted as part of that particular top-level Destroy() call.

> If the out-of-flow is destroyed first, it will unregister the placeholder and
> null out the placeholder's mOutOfFlowFrame (in nsFrame:Destroy); after that
> we're sort of on hook until the placeholder is destroyed.
> 
> Sound about right?

Right. Because of bug 492627 Patch Part IV, we don't need to have the placeholder-OOF link in order to remove frames, so this is safe.

I should assert in Part III that the placeholder is underneath aDestructRoot in this case. *does that*
>+    //XXXfr find a test that fails here
>
>I can't make sense of this comment....

That's just me noting that I wrote this bit as a correctness fix, but I don't actually have a test that shows the failure.

>>+    if (childType == nsGkAtoms::menuPopupFrame) {
>>+      listName = nsGkAtoms::popupList;
>
> That doesn't seem quite right...  If a <menu> has multiple <popup> kids,
> only the first one eds up on popupList.

Uh, okay... So, like this?
    if (childType == nsGkAtoms::menuPopupFrame) {
      nsIFrame* parent = aChildFrame->GetParent();
      firstPopup = (parent) ? parent->GetFirstChild(nsGkAtoms::menuPopupFrame);
      listName = (!firstPopup || firstPopup == aChildFrame)
                 ? nsGkAtoms::popupList
                 : nsnull;
    }
Can you sketch out a testcase that would trigger this?

> This is wrong for captions.  Then again, for captions we need to use a
> different parent too, right?  Should we just be asserting that captions never
> get passed in here?

Hm. I think we should handle captions, and use aChildFrame->GetParent() instead of manually tracking the parent in the cases where the frame is already inserted into the tree.

> Should this method just move to nsLayoutUtils, if it's generally useful? 

I don't mind.

> There's nothing frame constructor specific about it, right?

Not that I can see.

> Move the assert up a line if you want to keep it?

Good catch.

> Can the child list name for a placeholder ever be non-null?

I don't know. And I don't think I want to build that assumption in here.

> Can we assert something about the parent in the textFrame case?

We can assert something about aOldFrame's first-continuation's parent, but since we're deleting aOldFrame's chain in reverse order the number of operations would be... O(N^2), if I'm doing the math correctly.

So yes! We can assert something more specific here! But I'm not so sure we want to.

> So that's the main upshot of the changes to the primary frame stuff here,
> right?  I guess this is fine; I'm hoping to nuke the primary frame map anyway.

This replaces one of the main operations of DeletingFrameSubtree, yes. (The other one is making sure out-of-flow frames get deleted.)

> This looks wrong.

Ah, good point. We can screen out the case for continuation frames with
  if (!GetPrevContinuation())
It will fire for every continuation when we're deleting an entire chain, but at least it won't fire when we're deleting part of it (since we never delete the first continuation without deleting them all, IIRC). Are there other cases we need to screen out here--other cases where multiple frames share the same content node, but we're only deleting some of them?

> I just like less sibling-chain munging; it's error-prone. :)

Done.

> In other words, can we just assume oof->GetParent() here (and assert
> accordingly)?

Works for me.
>+   * Destroy() for each child). If this frame is a first-continuation, this
>+   * also unmaps the content associated with this frame.
>
> It's not really clear what unmaps means in this context.  Is that talking
> about the primary frame map?

It's talking about whatever
  RemoveAsPrimaryFrame(aFrame->GetContent(), aFrame);
  ClearAllUndisplayedContentIn(aFrame->GetContent());
does. If you have a better description, I'd appreciate the suggestion, as I'm not entirely clear what these do.
"Removes the frame from the primary frame map and clears undisplayed content for this content node." ?
> Can you sketch out a testcase that would trigger this?

<menu xmlns="whatever the xul namespace is">
  <popup/>
  <popup/>
</menu>

Your code for handling that looks ok.

> So yes! We can assert something more specific here! But I'm not so sure we
> want to.

OK.

> We can screen out the case for continuation frames with
>  if (!GetPrevContinuation())

Yeah, let's do that.  And then we can make this use the primary frame pointer, hopefully, when we nuke the primary frame map.

> Are there other cases we need to screen out here

Hmm.  First-letter frames being removed on DOM mutations, I think.  For now, that might be it, but maybe we should just land this after the primary frame changes, so we don't have to worry about all the ways this could mess up?
Comment on attachment 402294 [details] [diff] [review]
Patch Part III: Pass destruct root to frame Destroy methods

>+++ b/layout/generic/nsFrame.cpp
>+nsFrame::DestroyFrom(nsIFrame* aDestructRoot)
>-
>+ 

Don't add the whitespace, please.

>+    NS_ASSERTION(!(placeholder && (aDestructRoot == this)),
>+                 "Don't call Destroy() on OOFs, call Destroy() on the placeholder.");

I thinkg:

  !placeholder || aDestructRoot != this

would be more readable...

>+++ b/layout/generic/nsFrameList.h
>+  void DestroyFrames(nsIFrame* aDestructRoot = nsnull);

I'd really rather require aDestructRoot (with explicit null allowed, and documented, etc); that should make sure we catch all the callers.

>+  void Destroy(nsIFrame* aDestructRoot = nsnull);

Same here.

>+++ b/layout/generic/nsIFrame.h
>+   * Implements Destroy(). Do not call this directly except from within a
>+   * DestroyFrom() implementation.

How about just making DestroyFrom protected (and making nsFrameList a friend class as needed)?

aDestructRoot needs documentation.

>+++ b/layout/generic/nsLineBox.cpp
>+nsLineBox::DeleteLineList(nsPresContext* aPresContext, nsLineList& aLines,
>+      child->DestroyFrom((aDestructRoot) ? aDestructRoot : child);

How could aDestructRoot ever be null here?

>+++ b/layout/generic/nsLineBox.h
>+  static void DeleteLineList(nsPresContext* aPresContext, nsLineList& aLines,
>+                             nsIFrame* aDestructRoot = nsnull);

I don't think we should allow aDestructRoot to either be omitted or be null.

>+++ b/layout/generic/nsPlaceholderFrame.cpp
>+nsPlaceholderFrame::DestroyFrom(nsIFrame* aDestructRoot)
>+          (oof->GetStyleDisplay()->mDisplay == NS_STYLE_DISPLAY_POPUP ||

Once bug 518114 is fixed we should do a bit-check here.

>+      // else oof will be destroyed by its parent

This looks wrong to me.  In particular, consider a situation in which the placeholder is destroyed before the OOF (which is the only time we enter this code, right?).  We'd go through here and do nothing, then destroy the placeholder.  Then while destroying the OOF we'd hit nsFrame::Destroy, get the placeholder (now dead!) via GetPlaceholderFrameFor(), and call UnregisterPlaceholderFrame on it. Given frame poisoning, I expect that to crash more or less immediately, or at least totally fail to unregister the placeholder.  It might be that it's ok so far because all of our consumers in fact destroy the OOFs first... but that seems fragile.
Attachment #402294 - Flags: review?(bzbarsky) → review-
> I think  !placeholder || aDestructRoot != this would be more readable...

Fixed.

> How about just making DestroyFrom protected (and making nsFrameList a friend
> class as needed)? ... aDestructRoot needs documentation.

Done. Leaving the comment because the "do not call" applies to member functions as well as non-nsIFrame functions.

> I'd really rather require aDestructRoot 

Done.

> How could aDestructRoot ever be null here?

DestroyOverflowLines()

> This looks wrong to me.

Ah, thank you.

> Hmm.  First-letter frames being removed on DOM mutations, I think. 

Which frames in that case share an mContent?
Current state
Attachment #402293 - Attachment is obsolete: true
current state
Attachment #402294 - Attachment is obsolete: true
> the "do not call" applies to member functions as well as non-nsIFrame functions.

We might actually want to make it private if that will compile...

> DestroyOverflowLines()

Why can't that pass in a useful destruct root?

> Which frames in that case share an mContent?

The first-letter and its parent frame.
Ok, so I wasn't entirely sure what you wanted me to do about the first letter frames so I added a check for mContent and mParent->mContent being different. I don't think this logic was in the original DoDeletingSubtree, and I get crashes if I apply it to the RemoveAsPrimary call.
Attachment #414340 - Attachment is obsolete: true
Attachment #415064 - Flags: review?(bzbarsky)
Attachment #414341 - Flags: review?(bzbarsky)
> I wasn't entirely sure what you wanted me to do about the first letter frames 

I wasn't either; otherwise I would have told you what to do.  All I was saying is that the code as written will behave incorrectly.

mContent == mParent->mContent in all sorts of cases; one obvious one in which the |this| would in fact be the primary frame is table inner frames.  I'd think that those are most likely what accounts for your crashes.  I _think_ your modified code here is ok, but I'm not entirely sure.  :(

Any chance of interdiffs of your latest patches against whatever I reviewed?
Attachment #415136 - Attachment is patch: true
Attachment #415136 - Attachment mime type: application/octet-stream → text/plain
Blocks: 509749
Attachment #418278 - Attachment is patch: true
Attachment #418278 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 415064 [details] [diff] [review]
Patch Part II: Remove DeletingFrameSubtree

r=bzbarsky
Attachment #415064 - Flags: review?(bzbarsky) → review+
fantasai, did you ever answer my question from comment 27?
Comment on attachment 414341 [details] [diff] [review]
Patch Part III: Pass destruct root to frame Destroy methods

>+++ b/layout/generic/nsFrameList.h
>    * For each frame in this list: remove it from the list then call
>-   * Destroy() on it.
>+   * DestroyFrom() on it. If aDestructRoot is null, then call Destroy().

That makes it sounds like Destroy will get called on |this|.  Rephrase to make that clearer?

>    * For each frame in this list: remove it from the list then call
>-   * Destroy() on it. Finally <code>delete this</code>.
>+   * DestroyFrom() on it. If aDestructRoot is null, then call Destroy().

Likewise.

>+++ b/layout/generic/nsLineBox.h
>-  static void DeleteLineList(nsPresContext* aPresContext, nsLineList& aLines);
>+  static void DeleteLineList(nsPresContext* aPresContext, nsLineList& aLines,
>+                             nsIFrame* aDestructRoot);

I still think this should document that null is not allowed (and we should fix the one caller that passes null and simplify the implementation here).

>+++ b/layout/generic/nsPlaceholderFrame.cpp
>+nsPlaceholderFrame::DestroyFrom(nsIFrame* aDestructRoot)
>+        (GetStateBits() & PLACEHOLDER_FOR_FLOAT ||

I'd prefer parens about the '&' expression, for clarity.

r=me with those nits.
Attachment #414341 - Flags: review?(bzbarsky) → review+
> fantasai, did you ever answer my question from comment 27?

I thought I did, but apparently I didn't actually. Here goes:

> Why can't that pass in a useful destruct root?

Because afaict it's only called if we're tearing down the whole frame property table, or possibly under some error conditions. Where would we get a useful destruct root? Telling the frames to destroy themselves is safer than passing in a fake destruct root.

In the normal case, we're not calling DestroyOverflowLines: either the list is empty, or it gets removed from the proptable and then explicitly destroyed (with a destruct root) in nsBlockFrame::DestroyFrom.

Let me know if that addresses your concern, or, if not, what should be done to address it.
Blocks: 526217
> Because afaict it's only called if we're tearing down the whole frame property
> table, or possibly under some error conditions.

Aha.  Yes, ok.

Could we pass in aFrame as the destruct root in that case?  I guess that's not as safe, yeah.  So ignore that part for purposes of comment 35.
Attachment #414341 - Flags: superreview?(roc)
Attachment #415064 - Flags: superreview?(roc)
Attachment #393332 - Flags: superreview?(roc)
Attachment #393332 - Flags: superreview?(roc) → superreview+
Comment on attachment 414341 [details] [diff] [review]
Patch Part III: Pass destruct root to frame Destroy methods

   /**
    * For each frame in this list: remove it from the list then call
-   * Destroy() on it.
+   * DestroyFrom() on it. If aDestructRoot is null, then call Destroy().
    */
-  void DestroyFrames();
+  void DestroyFrames(nsIFrame* aDestructRoot);

I think it would be cleaner to just have two overloaded methods, separately implemented, where the no-arg one calls Destroy() and the one-arg one calls DestroyFrom().
Attachment #414341 - Flags: superreview?(roc) → superreview+
And the one that calls DestroyFrom should probably be called DestroyFramesFrom.
Attachment #415064 - Flags: superreview?(roc) → superreview+
Depends on: 536692
Blocks: 536718
Filed bug 536718 on making NotifyDestroyingFrame simpler now that bug 500882 has landed.
Depends on: 536721
There's even more dead code we can now get rid of: IsOutOfFlowList is now unused.
Depends on: 536720
We should get this on branch.
blocking1.9.2: --- → ?
status1.9.2: --- → ?
What's the risk of taking this on the branch? Would we need/want it on 1.9.1 as well? Do we expect the backport differences to be trivial?
It's a high-risk patch with a high payoff: it rewrites frame destruction in a way that's more sane and thereby fixed a number of crashers, including some that hadn't yet been detected when the patch was first written. So it's risky because it completely replaces existing frame destruction code with new code, but it's also risk-reducing because it makes those frame destruction tasks part of the frame tree's class architecture instead of wrapping external loops and recursion around it. It's had some time to stabilize on trunk, and there have been a few regressions whose fixes, for those that have them, would also need to be backported. I can't make a judgement call on whether and to what it should be backported--I defer to roc on that.
I think we should probably take it on the branch.
roc, I just remembered, I believe this patch as-written depends on bug 492627. So either we'd need to land that, too, or we'd need to modify this patch to handle placeholder continuations.
We need this on the branches, making it a blocker :-(

Please work on a 1.9.2 back-port, including a roll-up of the regression bugs and if needed bug 492627. Code-freeze for 1.9.2.8 will be sometime early-to-mid August.
blocking1.9.2: ? → .8+
For bug 576078?  I thought it was covered by frame poisoning for 3.6, and we hadn't yet decided how to address it for 3.5.
Yeah, I don't think it makes sense to make this a blocker unless we also think we can get it to 3.5.x in a straightforward way -- which seems unlikely.
Ok, you guys are the experts...marking as unblocking for the branches.
blocking1.9.2: .9+ → ---
This zipfile includes the following patches ported to 1.9.2.
  frame-destruct-I    Part I of this bug
  frame-destruct-II   Part II of this bug
  frame-destruct-III  Part III of this bug
  frame-destruct-fixup-I    Patch for bug 536721
  frame-destruct-fixup-II   Patch for bug 536720
  frame-destruct-fixup-III  Commenting fix for bug 536720

If you still want this, I will attach useful things for reviewing.
Depends on: 492627
Blocks: 737290
Depends on: 780985
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.