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

RESOLVED FIXED in mozilla1.9.1a2

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Haruka Iwao (Yuryu), Assigned: dbaron)

Tracking

({regression, verified1.9.0.2})

Trunk
mozilla1.9.1a2
regression, verified1.9.0.2
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 5 obsolete attachments)

6.68 KB, text/plain; charset=UTF-8
Details
3.74 KB, patch
Samuel Sidler (old account; do not CC)
: approval1.9.0.2+
Details | Diff | Splinter Review
3.70 KB, patch
Details | Diff | Splinter Review
44.68 KB, patch
surkov
: review+
Details | Diff | Splinter Review
29.36 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
This also reproduces on officially released Firefox 3 RC1.
(Reporter)

Comment 2

9 years ago
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
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.
Blocks: 401291
Component: General → Style System (CSS)
Keywords: qawanted, regression
Product: Firefox → Core
QA Contact: general → style-system
Version: unspecified → Trunk
Would be great with a minimal testcase.
(Assignee)

Comment 5

9 years ago
Created attachment 323145 [details]
stack showing the problem
(Assignee)

Comment 6

9 years ago
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 7

9 years ago
Created attachment 323161 [details] [diff] [review]
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?
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #323161 - Flags: superreview?(bzbarsky)
Attachment #323161 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
OS: Windows XP → All
Hardware: PC → All
IsNativeAnonymous() || GetBindingParent() can be simplified to just GetBindingParent(). Native anon content always has a binding parent iirc.

Comment 9

9 years ago
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

9 years ago
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.
Attachment #323161 - Flags: superreview?(bzbarsky)
Attachment #323161 - Flags: superreview+
Attachment #323161 - Flags: review?(bzbarsky)
Attachment #323161 - Flags: review+
(Assignee)

Comment 11

9 years ago
Triggering it isn't what I'm worried about as far as testing -- what I'm worried about is detecting the 100% cpu usage.
(Assignee)

Updated

9 years ago
Flags: wanted1.9.0.x?
(Assignee)

Comment 12

9 years ago
Created attachment 323923 [details] [diff] [review]
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?
(Assignee)

Comment 13

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

Comment 14

9 years ago
Created attachment 323962 [details] [diff] [review]
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?)
Attachment #323161 - Attachment is obsolete: true
Attachment #323923 - Attachment is obsolete: true
Attachment #323962 - Flags: superreview?(bzbarsky)
Attachment #323962 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

9 years ago
And please review the comments here carefully.
(Assignee)

Comment 16

9 years ago
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?
(Assignee)

Comment 17

9 years ago
Created attachment 323975 [details] [diff] [review]
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.
Attachment #323962 - Attachment is obsolete: true
Attachment #323962 - Flags: superreview?(bzbarsky)
Attachment #323962 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

9 years ago
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).
(Assignee)

Comment 19

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

Comment 20

9 years ago
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.
(Assignee)

Comment 21

9 years ago
Created attachment 324164 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent
(Assignee)

Comment 22

9 years ago
Created attachment 324165 [details] [diff] [review]
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.
Attachment #323975 - Attachment is obsolete: true
Attachment #324165 - Flags: superreview?(bzbarsky)
Attachment #324165 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

9 years ago
Note that the branch patch I just described is basically the already-reviewed attachment 323161 [details] [diff] [review].
(Assignee)

Comment 24

9 years ago
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
(Assignee)

Comment 25

9 years ago
Created attachment 324166 [details] [diff] [review]
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.
Attachment #324164 - Attachment is obsolete: true
Attachment #324166 - Flags: superreview?(bzbarsky)
Attachment #324166 - Flags: review?(bzbarsky)
(Assignee)

Comment 26

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

Comment 27

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

r+sr=bzbarsky
Attachment #324165 - Flags: superreview?(bzbarsky)
Attachment #324165 - Flags: superreview+
Attachment #324165 - Flags: review?(bzbarsky)
Attachment #324165 - Flags: review+

Comment 28

9 years ago
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?
Attachment #324166 - Flags: review?(aaronleventhal)
Attachment #324166 - Flags: review?(Olli.Pettay)

Updated

9 years ago
Attachment #324166 - Flags: review?(aaronleventhal) → review?(surkov.alexander)
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;
Attachment #324166 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 30

9 years ago
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.
(Assignee)

Comment 31

9 years ago
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

9 years ago
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

9 years ago
(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?
(Assignee)

Updated

9 years ago
Blocks: 425862
(Assignee)

Updated

9 years ago
Flags: in-testsuite?
(Assignee)

Updated

9 years ago
Attachment #323161 - Attachment description: patch → patch (for native anonymous content only)
Attachment #323161 - Attachment is obsolete: false
(Assignee)

Updated

9 years ago
Attachment #324165 - Attachment description: patch to fix this bug → patch to fix this bug (for all anonymous content)

Updated

9 years ago
Depends on: 439258

Comment 34

9 years ago
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.
Attachment #324166 - Flags: review?(surkov.alexander)
(Assignee)

Comment 35

9 years ago
(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?
(Assignee)

Updated

9 years ago
Attachment #324166 - Flags: review?(surkov.alexander)

Comment 36

9 years ago
(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

9 years ago
(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;
(Assignee)

Comment 38

9 years ago
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.
(Assignee)

Updated

9 years ago
Attachment #323161 - Flags: approval1.9.0.2?
(Assignee)

Updated

9 years ago
Blocks: 422276

Comment 39

9 years ago
(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.
(Assignee)

Updated

9 years ago
Attachment #323161 - Attachment description: patch (for native anonymous content only) → patch (for native anonymous content only) [landed on mozilla-central 2008-06-10]

Updated

9 years ago
Duplicate of this bug: 422276

Updated

9 years ago
Depends on: 444279
(Assignee)

Comment 41

9 years ago
(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.
(Assignee)

Comment 42

9 years ago
Created attachment 329265 [details] [diff] [review]
patch to change the rules of GetBindingParent and add new methods to nsIContent

This should address the review comments so far.
Attachment #324166 - Attachment is obsolete: true
Attachment #329265 - Flags: superreview?(bzbarsky)
Attachment #329265 - Flags: review?(bzbarsky)
Attachment #324166 - Flags: superreview?(bzbarsky)
Attachment #324166 - Flags: review?(surkov.alexander)
Attachment #324166 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
Attachment #329265 - Flags: review?(surkov.alexander)

Comment 43

9 years ago
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.
(Assignee)

Comment 44

9 years ago
Created attachment 330074 [details] [diff] [review]
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...
Attachment #330074 - Flags: superreview?(bzbarsky)
Attachment #330074 - Flags: review?(bzbarsky)

Comment 45

9 years ago
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.
Attachment #330074 - Flags: superreview?(bzbarsky)
Attachment #330074 - Flags: superreview+
Attachment #330074 - Flags: review?(bzbarsky)
Attachment #330074 - Flags: review+

Updated

9 years ago
Attachment #329265 - Flags: superreview?(bzbarsky)
Attachment #329265 - Flags: superreview+
Attachment #329265 - Flags: review?(bzbarsky)
Attachment #329265 - Flags: review+
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Keywords: qawanted

Comment 46

9 years ago
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

9 years ago
> 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

9 years ago
(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

9 years ago
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
Attachment #329265 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 50

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

Comment 51

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

Comment 52

9 years ago
(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
(Assignee)

Comment 53

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1
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?
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

9 years ago
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?
Any chance this fix caused https://bugzilla.mozilla.org/show_bug.cgi?id=447613
hmm, think I commented wrong bug...

(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.
(Assignee)

Comment 60

9 years ago
(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.
(Assignee)

Comment 61

9 years ago
And see comment 11 regarding tests.

Updated

9 years ago
Depends on: 447735
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
Attachment #323161 - Flags: approval1.9.0.2? → approval1.9.0.2+
(Assignee)

Comment 63

9 years ago
Landed attachment 323161 [details] [diff] [review] in CVS.
Keywords: fixed1.9.0.2
(Assignee)

Updated

9 years ago
Target Milestone: mozilla1.9.1 → mozilla1.9.1a2
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.
Keywords: fixed1.9.0.2 → verified1.9.0.2
You need to log in before you can comment on or make changes to this bug.