888981 - DownThemAll! causes browser to hang indefinitely

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Fanolian]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:25.0) Gecko/20130701 Firefox/25.0 (Nightly/Aurora)
Build ID: 20130701031115

Steps to reproduce:

1. In a new profile, install DownThemAll! 2.0.16. (https://addons.mozilla.org/en-US/firefox/addon/downthemall/). Restart browser.
2. Open dTa's manager via Menu > Tools.


Actual results:

Browser hangs indefinitely but does not crash. I can close the browser with the close button.


Expected results:

Nightly should not hang.

Last good Nightly: 2013-06-21
First bad Nightly: 2013-06-22

DownThemAll! 3.0b4 [1] does not hang the browser but v2.0.16 has about 1.7M users according to AMO. Many users may be affected when Firefox 24 is released.
[1]: Get the beta from http://www.downthemall.net/


Alternative way to reproduce without using extensions:
1. Enable error console by setting devtools.errorconsole.enabled to true. Restart browser.
2. Open error console via Web Developer > Error Console.
Browser Console, which will replace Error Console if I understand correctly, does not cause the hang.

Comment 1

[Alice0775 White]

I can reproduce the problem with the alternative str on desktop/Windows8.1 preview.

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/66bbf82b9ec2
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20130620 Firefox/24.0 ID:20130620165313
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2af3c33bfb29
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20130620 Firefox/24.0 ID:20130620170502
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=66bbf82b9ec2&tochange=2af3c33bfb29

Regressed by:
2af3c33bfb29	Sam Foster — Bug 828088 - Rework richgrid and richgriditem bindings to use css columns for down-then-across grids. r=fryn

Comment 2

[Frank Yan (:fryn)]

I'm confused. The changeset that you listed only touches Metro-specific code. How could it break desktop Firefox?
Are you sure that it wasn't the accessibility code change in the pushlog to which you linked?

Comment 3

[Alice0775 White]

(In reply to Frank Yan (:fryn) from comment #2)
> I'm confused. The changeset that you listed only touches Metro-specific
> code. How could it break desktop Firefox?
> Are you sure that it wasn't the accessibility code change in the pushlog to
> which you linked?

Then, it should be regressed by
b27220008ea6	Alexander Surkov — Bug 722265 - Column header selection popup no longer exposed to accessibility APIs, r=tbsaunde

Comment 4

[Alice0775 White]

stack using crashfirefox.exe
bp-fdec3797-dbb4-47ee-85d5-4f6312130701

Comment 5

[alexander :surkov (:asurkov)]

Trevor, I didn't looked at it yet. Did you have a chance over weekends?

Comment 6

[alexander :surkov (:asurkov)]

Last time when we chatted with Trevor, he debugged the problem so moving assignee status to him.

Comment 7

[bhavana bajaj [:bajaj]]

Needinfo'ing Trevor here , given we are one week away to stop take low risk patches for Fx24 and getting some action/next steps here.

Comment 8

[Alice0775 White]

I cannot reproduce the problem in Firefox 24.0b4 anymore.

http://hg.mozilla.org/releases/mozilla-beta/rev/995480afdf3e
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 ID:20130819170952

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to bhavana bajaj [:bajaj] from comment #7)
> Needinfo'ing Trevor here , given we are one week away to stop take low risk
> patches for Fx24 and getting some action/next steps here.

we shouldn't need to take anything on branches since we fixed bug 900067

Comment 10

[alexander :surkov (:asurkov)]

it still needs a patch to make sure it wont' trigger at other scenarios

Comment 11

[Trevor Saunders (:tbsaunde)]

not fixed, but no need to track anymore per comment 9

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: bug 888981 - add Accessible::IsAcceptableChild()

this patch does several things
- makes DocAccessible::BindToDocument return void the only case in which
  it would "fail" was when passed null, and we know none of the callers
do that.
- adds Accessible::IsAcceptableChild() which returns true if its ok for
  the possible child to be a child of this accessible.
- replace several over rides of Accessible::CacheChildren() that are
  only to prevent certain types of children to over rides of
Accessible::IsAcceptableChild()

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 800928 [details] [diff] [review]
bug 888981 - add Accessible::IsAcceptableChild()

Review of attachment 800928 [details] [diff] [review]:
-----------------------------------------------------------------

side comments
1) if accessible is not acceptable as a child then we will go into its kids to find accessible (current scenario is we move to next sibling), might be not important
2) role check is running before we bound the accessible to the document, the difference is ARIA role is not set up yet, might be not important
3) ideally we should have aContext->IsAcceptableType() to merge new IsAcceptableChild and aContext->IsOfType(), this patch is a good step on this way

r=me

::: accessible/src/generic/Accessible.h
@@ +435,5 @@
>  
>    /**
> +   * Return true if the accessible is an acceptable child.
> +   */
> +  virtual bool  IsAcceptableChild(Accessible* aPossibleChild) const { return true; }

nit: two spaces after bool

::: accessible/src/html/HTMLFormControlAccessible.h
@@ +142,5 @@
>  protected:
>    // Accessible
>    virtual ENameValueFlag NativeName(nsString& aName) MOZ_OVERRIDE;
>  
> +  virtual bool IsAcceptableChild(Accessible* aPossibleChild) const MOZ_OVERRIDE;

it's under protected section, should be under public

::: accessible/src/xul/XULColorPickerAccessible.h
@@ +46,5 @@
>    // Widgets
>    virtual bool IsWidget() const;
>    virtual bool IsActiveWidget() const;
>    virtual bool AreItemsOperable() const;
> +  virtual bool IsAcceptableChild(Accessible* aPossibleChild) const MOZ_OVERRIDE;

nit: new line before the method pls

::: accessible/src/xul/XULElementAccessibles.cpp
@@ +50,5 @@
>  
> +    nsAutoString text;
> +    textBoxFrame->GetCroppedTitle(text);
> +    mValueTextLeaf->SetText(text);
> +    return;

nit: you don't need return here

::: accessible/src/xul/XULFormControlAccessible.cpp
@@ -204,5 @@
> -    return;
> -
> -  AppendChild(menupopup);
> -  if (button)
> -    AppendChild(button);

side comment: the order of these accessibles in the tree might be changed (it must not make a difference)

::: accessible/src/xul/XULFormControlAccessible.h
@@ +50,5 @@
>    virtual bool IsWidget() const;
>    virtual bool IsActiveWidget() const;
>    virtual bool AreItemsOperable() const;
>    virtual Accessible* ContainerWidget() const;
> +  virtual bool IsAcceptableChild(Accessible* aPossibleChild) const MOZ_OVERRIDE;

nit: new line pls before IsAcceptableChild

::: accessible/src/xul/XULTreeAccessible.cpp
@@ +547,1 @@
>        return treeItem;

nit: wrong indentation of 'return' line

::: accessible/src/xul/XULTreeGridAccessible.cpp
@@ +404,3 @@
>    }
>  
>    return nullptr;

you may combine these returns by
return cell;

Comment 14

[Trevor Saunders (:tbsaunde)]

> 1) if accessible is not acceptable as a child then we will go into its kids
> to find accessible (current scenario is we move to next sibling), might be
> not important

maybe we should switch to enum like filters some day if we need it then

> 2) role check is running before we bound the accessible to the document, the
> difference is ARIA role is not set up yet, might be not important

for xul stuff I really doubt it matters, actually I sort of wish we could do it early before we even make accessible in the first place.

> ::: accessible/src/xul/XULFormControlAccessible.cpp
> @@ -204,5 @@
> > -    return;
> > -
> > -  AppendChild(menupopup);
> > -  if (button)
> > -    AppendChild(button);
> 
> side comment: the order of these accessibles in the tree might be changed
> (it must not make a difference)

yeah, but the new way seems better to me (always in dom order)

> ::: accessible/src/xul/XULTreeGridAccessible.cpp
> @@ +404,3 @@
> >    }
> >  
> >    return nullptr;
> 
> you may combine these returns by
> return cell;

actually I don't think we need if at all new never fails.

Comment 15

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a1bfde03d9

Comment 16

[alexander :surkov (:asurkov)]

you didn't update HyperTextAccessible::CacheChildren() to make IsAcceptableChild pattern,

btw, can you please put a short comment describing what the bug was and how the patch address it, I realized the discussion happened at irc but it'd be good to have it here for tracking proposes.

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> you didn't update HyperTextAccessible::CacheChildren() to make
> IsAcceptableChild pattern,

I figured it was safe to leave as is for now, we can fix more CacheChildren() impls later

> btw, can you please put a short comment describing what the bug was and how
> the patch address it, I realized the discussion happened at irc but it'd be
> good to have it here for tracking proposes.

So, if you create an accessible child and then try to bind it to the document we add dependant content that may need accessibles to mInvalidationList, but we don't remove that content if you immediately unbind that accessible because you don't actually want it as a child.  Then you go to handle mInvalidation list and just add more stuff to it because you keep thinking you need to maybe create accessibles.  The solution here is to allow accessibles to reject kids before we try and bind them to the document, that way we don't go looking for dependant content of kids we don't want and stuff doesn't get added to mInvalidationList.

Comment 18

[alexander :surkov (:asurkov)]

when we should call this bug fixed? Is it necessary to get rid all UnbdindFromDocument from ChacheChildren? Ideas how to protect from repeating this pattern in the future?

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> when we should call this bug fixed? Is it necessary to get rid all
> UnbdindFromDocument from ChacheChildren? Ideas how to protect from repeating
> this pattern in the future?

yeah, I guess it would be best if the only way UnbindFromDocument() was called was fromShutdownChildrenInSubtree()

When we should this bug fixed I'm not sure.

The rememaining UnbindFromDocument() callers are
HTMLImageMapAccessible.cpp (afaict unreachable InsertChildAt() can't actually fail)
HTMLListAccessible.cpp afaik the only reason that accessible needs to be bound to the document is for msaa child id stuff.
XULTreeAccessible.cpp same
XULFormControlAccessible.cpp we should convert to IsAcceptableChild() but probably need to keep CacheChildren() so it can use content that isn't mContent as root of tree to walk
HyperTextAccessible.cpp same accept cacheChildren() can go away

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4a1bfde03d9

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> (In reply to alexander :surkov from comment #18)
> > when we should call this bug fixed? Is it necessary to get rid all
> > UnbdindFromDocument from ChacheChildren? Ideas how to protect from repeating
> > this pattern in the future?
> 
> yeah, I guess it would be best if the only way UnbindFromDocument() was
> called was fromShutdownChildrenInSubtree()
> 
> When we should this bug fixed I'm not sure.
> 
> The rememaining UnbindFromDocument() callers are
> HTMLImageMapAccessible.cpp (afaict unreachable InsertChildAt() can't
> actually fail)

right, we never unbind if AppendChild fail so if we don't do this then probably we shouldn't do that for InsetChildAt

> HTMLListAccessible.cpp afaik the only reason that accessible needs to be
> bound to the document is for msaa child id stuff.
> XULTreeAccessible.cpp same

we could keep those

> XULFormControlAccessible.cpp we should convert to IsAcceptableChild() but
> probably need to keep CacheChildren() so it can use content that isn't
> mContent as root of tree to walk
> HyperTextAccessible.cpp same accept cacheChildren() can go away

that's right, would you like to file a short patch on it? Having fixed it we should be in a good shape