Last Comment Bug 730587 - Stash pointer to the subtree parent on nodes in disconnected subtrees
: Stash pointer to the subtree parent on nodes in disconnected subtrees
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on: 736695 745494
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-25 10:13 PST by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2012-06-21 13:06 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
Patch (20.20 KB, patch)
2012-02-27 12:27 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (14.92 KB, patch)
2012-02-27 13:16 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review+
jst: superreview+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-25 10:13:39 PST
Being able to always quickly get the subtree parent will make some CC optimizations more performant.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-27 12:27:23 PST
Created attachment 601012 [details] [diff] [review]
Patch
Comment 2 Olli Pettay [:smaug] 2012-02-27 12:47:33 PST
Comment on attachment 601012 [details] [diff] [review]
Patch


>-  nsIFrame* GetPrimaryFrame() const { return mPrimaryFrame; }
>+  nsIFrame* GetPrimaryFrame() const {
{ should be in the next line.

>+  void SetSubtreeRootPointer(nsINode* aSubtreeRoot) {
ditto

>+  void ClearSubtreeRootPointer() {
And here.

> nsINode::~nsINode()
> {
>   NS_ASSERTION(!HasSlots(), "nsNodeUtils::LastRelease was not called?");
>+  NS_ASSERTION(mSubtreeRoot == this, "Didn't restore state properly?");
> }
Use stronger MOZ_ASSERT here?
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-27 13:16:51 PST
Created attachment 601033 [details] [diff] [review]
Patch
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-27 13:18:53 PST
(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 601012 [details] [diff] [review]
> Patch
> 
> 
> >-  nsIFrame* GetPrimaryFrame() const { return mPrimaryFrame; }
> >+  nsIFrame* GetPrimaryFrame() const {
> { should be in the next line.

Done.

> >+  void SetSubtreeRootPointer(nsINode* aSubtreeRoot) {
> ditto

Done.

> >+  void ClearSubtreeRootPointer() {
> And here.

Done.

> > nsINode::~nsINode()
> > {
> >   NS_ASSERTION(!HasSlots(), "nsNodeUtils::LastRelease was not called?");
> >+  NS_ASSERTION(mSubtreeRoot == this, "Didn't restore state properly?");
> > }
> Use stronger MOZ_ASSERT here?

It seems odd to have one fatal assert when we use NS_ASSERTION all over the DOM code.
Comment 5 Olli Pettay [:smaug] 2012-02-27 13:24:48 PST
Well, we haven't had MOZ_ASSERT for a long time.
In this case I really want debug builds to crash asap if something goes wrong.
Comment 6 Olli Pettay [:smaug] 2012-02-27 13:29:08 PST
Comment on attachment 601033 [details] [diff] [review]
Patch

>+  nsIFrame* GetPrimaryFrame() const
>+  {
>+    return IsInDoc() ? mPrimaryFrame : nsnull;
I wonder if this shows up in the profiles. Probably not.


>+  union {
I think { should be in the next line.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2012-03-08 09:45:20 PST
Comment on attachment 601033 [details] [diff] [review]
Patch

Looks good.
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-14 13:16:00 PDT
http://hg.mozilla.org/mozilla-central/rev/5280e98d2d77
Comment 9 Daniel Holbert [:dholbert] 2012-03-16 15:20:46 PDT
Pushed a followup to make OwnerDocAsNode non-inline, with rs=khuey:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f2bacfc9013c
Comment 10 Phil Ringnalda (:philor, back in August) 2012-03-17 17:31:50 PDT
https://hg.mozilla.org/mozilla-central/rev/f2bacfc9013c
Comment 11 Boris Zbarsky [:bz] 2012-04-30 07:26:02 PDT
We should consider backing this out of Aurora 14 due to bug 736695.
Comment 12 Alex Keybl [:akeybl] 2012-04-30 15:45:18 PDT
Although backing this out will probably end up being the solution, we'll instead continue to track bug 736695 since it's the actual regression.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-07 12:31:06 PDT
Backed out of 14 to fix bug 736695.

https://hg.mozilla.org/releases/mozilla-aurora/rev/abe9bb783f88
Comment 14 Boris Zbarsky [:bz] 2012-06-08 00:33:31 PDT
We need to back this out of Aurora 15 as well....
Comment 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-15 17:29:40 PDT
We have this backout tracked already in bug 736695 so not tracking here.
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-21 01:06:54 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/44103bdf65b1
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-21 13:06:53 PDT
And once more, with feeling: https://hg.mozilla.org/releases/mozilla-aurora/rev/0f805ac53831

Note You need to log in before you can comment on or make changes to this bug.