436453 - Firefox freezes with 100% cpu usage when I load the page

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emma Haruka Iwao]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052907 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052907 Minefield/3.0pre

After loading 
http://firefox.geckodev.org/index.php?Tab%20Mix
Firefox uses 100% cpu.
The browser seems to keep working, but some features including right-clicking
on the page, text boxes and check boxes are not working.
This also reduces system performance (and battery lifetime for laptop pc).


Reproducible: Always

Steps to Reproduce:
1. Load http://firefox.geckodev.org/index.php?Tab%20Mix
2.
3.
Actual Results:  
Firefox freezes with 100% cpu usage.

Expected Results:  
Idle cpu after the load is complete.

Comment 1

[Emma Haruka Iwao]

This also reproduces on officially released Firefox 3 RC1.

Comment 2

[Emma Haruka Iwao]

Sorry for the spam,

This works fine on
Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.8.1.14) Gecko/20080404 Firefox/2.0.0.14

Comment 3

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008053006 Minefield/3.0pre

Regression range is 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1203396840&maxdate=1203403139
and I think it is one of the bugs "Fix handling of dynamic changes for advanced CSS selectors". 
When the page is in the cache, the next browser session, the hang doesn't happen anymore.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Would be great with a minimal testcase.

Comment 5

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

Attachment: stack showing the problem

Comment 6

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

The problem here is that doing a restyle causes counters to be recomputed, which causes a CharacterDataChanged notification that in turn causes a restyle.

I think we should probably just ignore CharacterDataChanged (and probably other) notifications on anything not actually in the document (which hopefully matches some easily testable definition of anonymous content).

Comment 7

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

Attachment: patch (for native anonymous content only) [landed on mozilla-central 2008-06-10]

This doesn't do the same thing for the XBL case (there's no IsAnonymous; sicking says I could use IsNativeAnonymous() || GetBindingParent()), but I don't think we can hit the infinite loop case for that.  Perhaps I should anyway, though.

Any bright ideas on how to test this?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IsNativeAnonymous() || GetBindingParent() can be simplified to just GetBindingParent(). Native anon content always has a binding parent iirc.

Comment 9

[Boris Zbarsky [:bzbarsky]]

As far as testing, we'd need something that triggers one of those selector flags, right?  And anonymous counter content on it?  I'd think that should hit it.

Checking GetBindingParent() is not good enough.  Or rather it'll have false positives, since it will fire for XBL-bound content that's just in the middle of the XBL subtree, and which can easily trigger these notifications...  The right check is really "is this node a root of a native anonymous subtree", and that check is currently best expressed as 

  IsNativeAnonymous() || 
  (GetBindingParent() && GetBindingParent() == GetParent())

or some such...  Kinda annoying, to be honest.  Maybe we should add an nsIContent method for checking this.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 323161 [details] [diff] [review]
patch (for native anonymous content only) [landed on mozilla-central 2008-06-10]

Looks good either this way or with the slightly more extensive check.

Comment 11

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

Triggering it isn't what I'm worried about as far as testing -- what I'm worried about is detecting the 100% cpu usage.

Comment 12

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

Attachment: patch with nsIContent::IsAnonymous, etc.

I think doing something like this would make things a bit easier to understand.

However, this patch triggers my assertions because the br inside the anonymous div inside an HTML text input has a GetBindingParent() of the div, yet it is in the div's child list (as the only child).

What's wrong here?

Comment 13

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

OK, so it seems like the rules for GetBindingParent are different for native-anonymous content, as asserted at the start of nsGenericElement::BindToTree.

Comment 14

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

Attachment: patch

So this records what I think I've learned in the form of code.  I've added two inline methods to nsIContent, although I only use one of them.

(Any ideas on how to test for this 100%-CPU condition?)

Comment 15

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

And please review the comments here carefully.

Comment 16

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

That said, it occurs to me that if it's possible to apply an XBL binding to native anonymous content nodes (not their descendants, but the native-anonymous node itself), then it's impossible to actually tell what content is not in it's parent's child list without actually going through that child list, using these APIs.  (Do we do that for scrollbars?)

Or would we not propagate the in-native-anonymous-subtree bit into the anonymous content in that case?

Comment 17

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

Attachment: patch

I think it really needs to be more like this... except this fails the assertion I added in ContentRemoved in the same case I mentioned before, because this IsAnonymous doesn't work in a ContentRemoved notification (it claims the BR node is anonymous when it is really not).  It's also really ugly.

Comment 18

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

bz, I think I'm stuck here.  Any thoughts?  I basically need the method above, but I want it to work in ContentRemoved too (which is before UnbindFromTree).

Comment 19

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

I suppose one solution would be adding a SetAnonymous that sets a bit, called by SetNativeAnonymous and by other places.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I think it would make a lot of sense to have a bit that marks anonymous subtree roots, yeah.

Since the ContentRemoved is after the node is gone from the parent's child list, the anonymous state really has to be on the node itself, somewhere.  If we didn't have the bindingParent hack on native anonymous subtree roots (e.g. if it were null, or set to the non-anonymous parent, or something), we could just use the bindingParent.... But then we'd need another way to detect native anonymous subtree roots.

Arguably, that's the cleanest thing to do in some ways.  Make bindingParent behavior the same for XBL-bound and native-anonymous content, and add IsNativeAnonymous() checks in places where we currently use the |GetBindingParent() == this| thing.  It used to be that we couldn't do that because the XUL IsNativeAnonymous() was broken, but we've fixed that now.

Comment 21

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

Attachment: patch to change the rules of GetBindingParent and add new methods to nsIContent

Comment 22

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

Attachment: patch to fix this bug (for all anonymous content)

If we want to land this on a branch without attachment 324164 [details] [diff] [review], it should just have s/Anonymous/NativeAnonymous/, which will fix all known cases of this bug, although not necessarily all theoretically possible cases.

Comment 23

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

Note that the branch patch I just described is basically the already-reviewed attachment 323161 [details] [diff] [review].

Comment 24

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

Comment on attachment 324164 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent

Some issues to consider:

 * I'm inclined to remove the initial native-anonymous check in IsAnonymous/IsInAnonymousSubtree.

 * I'm a little unhappy with the names of IsAnonymous / IsNativeAnonymous.  I think something like IsRootOf(Native)?AnonymousSubtree would be better, although it's awfully long.  Having gone through existing callers of GetBindingParent, I'm really not sure if some of them meant "IsIn" or "IsRootOf".  Thoughts on naming here?

 * I went through all callers of GetBindingParent; I realize I need to go through all callers of BindToTree, and that I missed some, so not requesting review yet

Comment 25

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

Attachment: patch to change the rules of GetBindingParent and add new methods to nsIContent

I'd only missed one, and I cross-checked the quick BindToTree full-tree audit with a SetNativeAnonymous audit (much fewer callers, and I had them all already).

See previous comment for review questions about this patch.

Comment 26

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

Full regular, chrome (except for nondeterminism in test_bug343416.xul), and browser chrome mochitests pass.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 324165 [details] [diff] [review]
patch to fix this bug (for all anonymous content)

r+sr=bzbarsky

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 324166 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent

I'd like to have Aaron take a look at the accessibility changes, especially the XXX comments.

>+++ b/content/base/public/nsIContent.h

>+  PRBool IsAnonymous() const
>+    // (Is that worth it?  Is it likely to be true a substantial portion
>+    // of the time?)

The answer to the latter question is probably "no".... But hard to say, since there are no actual uses of this function yet.

>+  PRBool IsInAnonymousSubtree() const
>+    // Check IsInNativeAnonymousSubtree first only because it's faster;
>+    // GetBindingParent() alone should be equivalent.
>+    // (Is that worth it?  Is it likely to be true a substantial portion
>+    // of the time?)

I suspect the answer for the second question is, again, "no".

>+   * Gets content node with the binding (or native code, possibly on the
>+   * frame) responsible for our construction (and existence).  Used by
>+   * anonymous content (XBL-generated).

s/XBL-generated/both XBL-generated and native anonymous/ or something?

>+++ b/content/base/src/nsDocument.cpp
>+    // XXXldb: Faster to jump to GetBindingParent if non-null?

Yes.  Followup bug?

>+++ b/content/base/src/nsGenericDOMDataNode.cpp
>+                 // REVIEW: I think this check is better now than it was
>+                 // before, but I'm not sure.

Since the assert never fires if IsNativeAnonymous(), and we didn't change the binding parent for nodes for which that check is false, I don't think anything changed here.

>-    if (IsNativeAnonymous() ||
>-        aBindingParent->IsInNativeAnonymousSubtree()) {
>+    if (IsNativeAnonymous() || aParent->IsInNativeAnonymousSubtree()) {

This looks right to me, but it'd be nice if smaug would take a look too.

> nsIContent::FindFirstNonNativeAnonymous() const

This function's behavior might be changed by this patch.  If I have a node A which is in the subtree rooted at at native anonymous node B, and A has an XBL binding attached, which has a non-root node C in the <content>, and C has native anonymous content bound to it, then the old code would return the first non-native-anonymous ancestor of B if |this| is in the anonymous content of C.  Will the new code do the same, or will it return C?  I don't recall exactly how the native anonymous subtree flag propagation works.

>@@ -2021,21 +2018,20 @@ nsGenericElement::BindToTree(nsIDocument

All the same comments as for nsGenericDOMDataNode apply.

>+++ b/content/xbl/src/nsBindingManager.cpp
>       break; // The anonymous content case is often deliberately hacked to
>              // return itself to cut off style inheritance here.  Do that.

This comment isn't quite right, since there is no such hack anymore.

>+++ b/content/xbl/src/nsXBLService.cpp
>@@ -105,20 +105,19 @@ IsAncestorBinding(nsIDocument* aDocument
>                   nsIURI* aChildBindingURI,
>                   nsIContent* aChild)

>+  for (nsIContent *prev = aChild, *bindingParent = aChild->GetBindingParent();
>+       bindingParent;
>        prev = bindingParent, bindingParent = bindingParent->GetBindingParent()) {

|prev| was only used in the loop condition, so now it's write-only.  We should just ditch it.

>+++ b/layout/base/nsCSSFrameConstructor.cpp

> #ifdef MOZ_SVG
>     // least-surprise CSS binding until we do the SVG specified
>     // cascading rules for <svg:use> - bug 265894

It looks like <svg:use>-bound stuff should be unaffected by this patch, since we're not calling SetNativeAnonymous() on it, before or after, so it keeps getting styled as before, right?  In any case, I'd hope we have tests for it that would trigger if there is an issue here...

>@@ -10765,18 +10760,18 @@ IsBindingAncestor(nsIContent* aContent, 
>+    // FOR REVIEW: Old code here seems broken, actually; should have
>+    // stepped to GetParent.

Hmm.  What this code is trying to detect is a case when a non-anonymous kid's frame is actually in a subtree rooted by some anonymous kid.  So when finding the frame for a given non-anonymous kid, we look at all the immediate child frames whose content is not anonymous wrt us and recurse down into the ones which correspond to anonymous content, since there might be non-anonymous kids down there, in insertion points.

So I think stepping up to the bindingParent is ok, isn't it?

Also, this code used to not recurse down into native anonymous content, and with this change it will.  I don't think that changes correctness (since we'll never find what we're looking for under there), and shouldn't affect performance much, if at all, so it's probably fine.

>@@ -11852,17 +11847,18 @@ nsCSSFrameConstructor::CreateFloatingLet
>+  // XXXldb: Should we really assert this?

For now, yes, because if it ever happens we'll screw up the frame tree in various ways by calling ContentInserted/etc on the native anonymous content.

>@@ -11985,17 +11981,18 @@ nsCSSFrameConstructor::CreateLetterFrame

Same here.

As far as comment 24 goes, I agree with your inclination in the first bullet point.   I'd be happy with the awfully long name, I think.  It would certainly make it clearer what's being checked!  Perhaps s/Anonymous/Anon/ if we want to shorten it?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 324166 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent

With this change the event re-targeting can be simplified a bit - no need to special case
native anon content. But that is a different bug.

>-    if (IsNativeAnonymous() ||
>-        aBindingParent->IsInNativeAnonymousSubtree()) {
>+    if (IsNativeAnonymous() || aParent->IsInNativeAnonymousSubtree()) {
>       SetFlags(NODE_IS_IN_ANONYMOUS_SUBTREE);
>     }
Looks ok to me. Whenever a content object is bind to native anonymous parent, it is marked
to be in native anon subtree.

>+  for (const nsIContent *content = this; content;
>+       content = content->GetParent()) {
Could you use here GetBindingParent() to get faster out of native anonymous subtree?
>+    if (!content->IsInNativeAnonymousSubtree()) {
>+      // Oops, this function signature allows casting const to
>+      // non-const.  (Then again, so does GetChildAt(0)->GetParent().)
>+      return const_cast<nsIContent*>(content);
>+    }
>+  }
>+  return nsnull;

Comment 30

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

Still need to go through all these comments; however, I realize I should probably land attachment 323161 [details] [diff] [review] on trunk soon, so that I can request 1.9.0.1 approval for it, and then I'll land the newer patch on top of it.

Comment 31

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

I landed the patch that I want for 1.9.0.* on mozilla-central so it gets testing:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/4cf8d09a71cf

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 324166 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent


>--- a/accessible/src/base/nsAccessibilityUtils.cpp
>+++ b/accessible/src/base/nsAccessibilityUtils.cpp
>@@ -748,17 +748,17 @@ nsAccUtils::FindNeighbourPointingToNode(
>                                         PRUint32 aAttrNum,
>                                         nsIAtom *aTagName,
>                                         PRUint32 aAncestorLevelsToSearch)
> {
>   nsCOMPtr<nsIContent> binding;
>   nsAutoString controlID;
>   if (!nsAccUtils::GetID(aForNode, controlID)) {
>     binding = aForNode->GetBindingParent();
>-    if (binding == aForNode)
>+    if (aForNode->IsNativeAnonymous()) // XXX Was this the intent?
>       return nsnull;
> 
>     aForNode->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::anonid, controlID);
>     if (controlID.IsEmpty())
>       return nsnull;
>   }

This block means 'aForNode' hasn't ID attribute and we check if it is inside anonymous content and then check 'anonid' attribute. iirc GetBindingParent() may return itself and that means 'aForNode' is not inside anonymous content. Here I think you should place the following check:

if (!aForNode-> IsInAnonymousSubtree())
  return nsnull;

That should mean 'aForNode' isn't inside anonymous content, right?

>   // Can get text from title of <toolbaritem> if we're a child of a <toolbaritem>
>   nsIContent *bindingParent = content->GetBindingParent();
>+  // XXXldb: Why do we skip to bindingParent's parent?
>   nsIContent *parent = bindingParent? bindingParent->GetParent() :
>                                       content->GetParent();

I'm not sure too why we do this so XXX comment must be fine here.

the rest part of accessibility module changes look good for me.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)


> >-    if (binding == aForNode)

> if (!aForNode-> IsInAnonymousSubtree())
>   return nsnull;

Though it should be equivalent things  but why do you want to change this?

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 324166 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent

clearing request unti comment 32 and comment 33 are addressed.

Comment 35

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

(In reply to comment #33)
> > >-    if (binding == aForNode)
> 
> > if (!aForNode-> IsInAnonymousSubtree())
> >   return nsnull;
> 
> Though it should be equivalent things  but why do you want to change this?

The reason I want to change this is because I'm changing what GetBindingParent returns for native anonymous content.  Before this patch, a native anonymous node would return itself as its GetBindingParent.  After this patch, it returns its parent.  So the change I was making keeps the equivalent behavior (whereas not making a change would have changed the behavior).

If you think the behavior is wrong, could you file a followup bug, and I can cite that bug number in the comment?

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> The reason I want to change this is because I'm changing what GetBindingParent
> returns for native anonymous content.  Before this patch, a native anonymous
> node would return itself as its GetBindingParent.  After this patch, it returns
> its parent.  So the change I was making keeps the equivalent behavior (whereas
> not making a change would have changed the behavior).
>

I don't get why it's equivalent. If previously we would stop if the 'aForNode' is not anonymous (GetBindingParent returns itself) then now we will stop if 'aForNode' is anonymous.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #36)
> (In reply to comment #35)
> > The reason I want to change this is because I'm changing what GetBindingParent
> > returns for native anonymous content.  Before this patch, a native anonymous
> > node would return itself as its GetBindingParent.  After this patch, it returns
> > its parent.  So the change I was making keeps the equivalent behavior (whereas
> > not making a change would have changed the behavior).
> >
> 
> I don't get why it's equivalent. If previously we would stop if the 'aForNode'
> is not anonymous (GetBindingParent returns itself) then now we will stop if
> 'aForNode' is anonymous.
> 

I mean the code in the patch:

>-    if (binding == aForNode)
>+    if (aForNode->IsNativeAnonymous()) // XXX Was this the intent?
>       return nsnull;

Comment 38

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

The point is that this patch is changing what GetBindingParent returns to be more consistent.  GetBindingParent on a node will now never return itself.  It previously returned itself in exactly the cases where IsNativeAnonymous returned true.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)
> The point is that this patch is changing what GetBindingParent returns to be
> more consistent.  GetBindingParent on a node will now never return itself.  It
> previously returned itself in exactly the cases where IsNativeAnonymous
> returned true.
> 

So it looks correct but I still have doubts concerning logic of changed code. Let me write some mochitests before review of the patch to test it.

Comment 41

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

(In reply to comment #28)
> > nsIContent::FindFirstNonNativeAnonymous() const
> 
> This function's behavior might be changed by this patch.  If I have a node A
> which is in the subtree rooted at at native anonymous node B, and A has an XBL
> binding attached, which has a non-root node C in the <content>, and C has
> native anonymous content bound to it, then the old code would return the first
> non-native-anonymous ancestor of B if |this| is in the anonymous content of C. 
> Will the new code do the same, or will it return C?  I don't recall exactly how
> the native anonymous subtree flag propagation works.

The new code will do the same; nsGenericElement::BindToTree propagates the flag if the parent has the flag.

Comment 42

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

Attachment: patch to change the rules of GetBindingParent and add new methods to nsIContent

This should address the review comments so far.

Comment 43

[Boris Zbarsky [:bzbarsky]]

David, if you can easily create an interdiff with the patch I already reviewed, can you do that?  That would help me out a lot...  If it's too much hassle, I guess I can review the new thing in its entirety.

Comment 44

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

Attachment: interdiff between patches (excluding merging)

This is an interdiff, generated out of the history of my queue repository.  (Hooray for having a versioned queue repository.  I popped all my patches off, updated my tree to the revision that it was at when I did the addressing of review comments, updated the queue repository to the version before I started addressing the review comments, pushed the patch on, then unapplied the patch and applied the one from the version of the queue repository after the comments were addressed, and diffed the tree.  Then I reversed the application of the patches, popped everything off, and updated both repositories back to tip.)

Somehow I managed to miss your comment when you made it...

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 330074 [details] [diff] [review]
interdiff between patches (excluding merging)

r+sr=bzbarsky.  Thank you for making the interdiff; that made it really easy to check the things from my previous review and make sure everything else was just the name change.

Comment 46

[alexander :surkov (:asurkov)]

Comment on attachment 329265 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent


>   nsCOMPtr<nsIContent> binding;
>   nsAutoString controlID;
>   if (!nsAccUtils::GetID(aForNode, controlID)) {
>     binding = aForNode->GetBindingParent();

It sounds it doesn't make sense to get 'binding' here because we may return early.

>-    if (binding == aForNode)
>+    if (aForNode->IsRootOfNativeAnonymousSubtree()) // XXX Was this the intent?
>       return nsnull;

'aForNode' hasn't ID attribute so we shouldn't do anything if the node is not anonymous because it that case we don't take into accunt 'anonid' attribute. Would you like to put the usual comment here instead of XXX one or remove it at all? :)

Please give me an example for this case, i.e. I need an element where IsRootOfNativeAnonymousSubtree() is true. Or is it true for every bound element?

>   // Can get text from title of <toolbaritem> if we're a child of a <toolbaritem>
>   nsIContent *bindingParent = content->GetBindingParent();
>+  // XXXldb: Why do we skip to bindingParent's parent?

That's correct I guess. You don't need XXX section here. Actually anonymous content of child of the toolbaritem gets the name from @title attribute of toolbaritem. imo It's disputable thing but we decided to not wake up sleeping dog.

Comment 47

[Boris Zbarsky [:bzbarsky]]

> I need an element where IsRootOfNativeAnonymousSubtree() is true.

A scrollbar on a <div style="overflow: scroll">.  The <div> inside an <input type="text">.  The image for CSS that says "content: url(...)".  There are probably other cases too.

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #46)

> >-    if (binding == aForNode)
> >+    if (aForNode->IsRootOfNativeAnonymousSubtree()) // XXX Was this the intent?
> >       return nsnull;
> 
> 'aForNode' hasn't ID attribute so we shouldn't do anything if the node is not
> anonymous because it that case we don't take into accunt 'anonid' attribute.
> Would you like to put the usual comment here instead of XXX one or remove it at
> all? :)

per comment #38 here should be

if (!aForNode->IsInAnonymousSubtree())
  return nsnull;

Comment 49

[alexander :surkov (:asurkov)]

Comment on attachment 329265 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent

r=me with addressed comment #46 and comment #48

Comment 50

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

I really don't want to make additional behavior changes in this patch.  Can we make those in followup bugs instead, please?

Comment 51

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

Well, I suppose I can push them in a second patch as part of the same bug.

Comment 52

[alexander :surkov (:asurkov)]

(In reply to comment #50)
> I really don't want to make additional behavior changes in this patch.  Can we
> make those in followup bugs instead, please?
> 

(In reply to comment #51)
> Well, I suppose I can push them in a second patch as part of the same bug.
> 

for sure it's up to you

Comment 53

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

Remaining patches checked in:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ce6c80430400
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ba8e7ded433b
http://hg.mozilla.org/mozilla-central/index.cgi/rev/80865e03f6af

Comment 54

[Samuel Sidler (old account; do not CC)]

David, we're not ready for this on 1.9.0 (needs to bake and needs a test) but can you provide a complete patch that applies to the 1.9.0 branch?

Comment 55

[Marco Zehe (:MarcoZ)]

This breaks three tests in accessible/tests/mochitest/test_nsIAccessible_name.xul:

not ok - Wrong label for anonymous textbox of box_label_anon1 got "", expected "Label"
not ok - Wrong label for anonymous textbox of box_label_anon2 got "", expected "Label"
not ok - Wrong label for anonymous textbox of box_label_anon2 got "", expected "Top textbox".

Do the tests need adjusting, or do we have a real breackage here?

Comment 56

[alexander :surkov (:asurkov)]

I believe the reason is http://hg.mozilla.org/mozilla-central/index.cgi/rev/ba8e7ded433b

-    if (aForNode->IsRootOfNativeAnonymousSubtree()) // XXX Was this the intent?
      12 +    if (aForNode->IsInAnonymousSubtree())
      13        return nsnull;

Should be 

if (!aForNode->IsInAnonymousSubtree())

Marco, could you try?
David, could you fix?

Comment 57

[Jim Jeffery not reading bug-mail 1/2/11]

Any chance this fix caused https://bugzilla.mozilla.org/show_bug.cgi?id=447613

Comment 58

[Jim Jeffery not reading bug-mail 1/2/11]

hmm, think I commented wrong bug...

Comment 59

[Marco Zehe (:MarcoZ)]

(In reply to comment #56)
> I believe the reason is
> http://hg.mozilla.org/mozilla-central/index.cgi/rev/ba8e7ded433b
> 
> -    if (aForNode->IsRootOfNativeAnonymousSubtree()) // XXX Was this the
> intent?
>       12 +    if (aForNode->IsInAnonymousSubtree())
>       13        return nsnull;
> 
> Should be 
> 
> if (!aForNode->IsInAnonymousSubtree())
> 
> Marco, could you try?

Yes this fixes the tests.

Comment 60

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

(In reply to comment #54)
> David, we're not ready for this on 1.9.0 (needs to bake and needs a test) but
> can you provide a complete patch that applies to the 1.9.0 branch?

The patch for the 1.9.0 branch has been baking since June 10.  See comment 31.

Comment 61

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

And see comment 11 regarding tests.

Comment 62

[Samuel Sidler (old account; do not CC)]

Comment on attachment 323161 [details] [diff] [review]
patch (for native anonymous content only) [landed on mozilla-central 2008-06-10]

Whoops. Sorry David. I read through the bug the first time but was too quick on the trigger last time. :-/

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 63

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

Landed attachment 323161 [details] [diff] [review] in CVS.

Comment 64

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in 1.9.0.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2) Gecko/2008090212 Firefox/3.0.2.