DownThemAll! causes browser to hang indefinitely

RESOLVED FIXED in mozilla27

Status

()

--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Fanolian+bugzilla, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug, {hang, regression})

24 Branch
mozilla27
x86
Windows 8
hang, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24 affected, firefox25 affected)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

5 years ago
Keywords: regression

Comment 1

5 years ago
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
Blocks: 828088
Severity: normal → critical
Status: UNCONFIRMED → NEW
status-firefox23: --- → unaffected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
Ever confirmed: true
Keywords: hang

Comment 2

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

5 years ago
(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
Blocks: 722265
No longer blocks: 828088

Comment 4

5 years ago
stack using crashfirefox.exe
bp-fdec3797-dbb4-47ee-85d5-4f6312130701
Trevor, I didn't looked at it yet. Did you have a chance over weekends?

Updated

5 years ago
Assignee: nobody → surkov.alexander
tracking-firefox24: ? → +
tracking-firefox25: ? → +
Last time when we chatted with Trevor, he debugged the problem so moving assignee status to him.
Assignee: surkov.alexander → trev.saunders

Updated

5 years ago
Component: Untriaged → Disability Access APIs
Product: Firefox → Core
Hardware: x86_64 → x86
Depends on: 900097
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.
Flags: needinfo?(trev.saunders)

Comment 8

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

Comment 9

5 years ago
(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
Flags: needinfo?(trev.saunders)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
it still needs a patch to make sure it wont' trigger at other scenarios
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 11

5 years ago
not fixed, but no need to track anymore per comment 9
tracking-firefox24: + → ---
tracking-firefox25: + → ---
(Assignee)

Comment 12

5 years ago
Created attachment 800928 [details] [diff] [review]
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()
Attachment #800928 - Flags: review?(surkov.alexander)
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;
Attachment #800928 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 14

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

Comment 17

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

Comment 19

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/a4a1bfde03d9
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(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
(Assignee)

Updated

5 years ago
Blocks: 72226, 888531
No longer blocks: 722265
You need to log in before you can comment on or make changes to this bug.