412878 - Crash [@ nsAccessible::InvalidateChildren ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[Aaron Leventhal]

This is a fairly common crash now. The first time it shows up on crash stats is Jan 10 builds, although that could just be the first time Firefox 3 builds started getting heavy usage.

http://crash-stats.mozilla.com/report/list?range_unit=months&query_search=stack&query_type=contains&version=Firefox%253A3.0b3pre&signature=nsAccessible%253A%253AInvalidateChildren()&query=InvalidateChildren&range_value=2

I see several sub-types, in both cases the doc has finished loading and FireDocLoadEvents() wants to be sure to invalidate any children that were created while it was still loading (e.g. the user tabbed into the doc so an accessible needed to be created for the focus event).

Here is when TryFireEarlyLoadEvent() succeeded and EVENT_INTERNAL_LOAD was fired.

0 nsAccessible::InvalidateChildren()  	 mozilla/accessible/src/base/nsAccessible.cpp:541
1 nsDocAccessible::FireDocLoadEvents(unsigned int) 	mozilla/accessible/src/base/nsDocAccessible.cpp:779
2 nsAccessibilityService::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, unsigned int) 	mozilla/accessible/src/base/nsAccessibilityService.cpp:219
3 nsDocLoader::FireOnStateChange(nsIWebProgress*, nsIRequest*, int, unsigned int) 	mozilla/uriloader/base/nsDocLoader.cpp:1235

This is from the later doc load finished handler:

0 nsAccessible::InvalidateChildren()  	 mozilla/accessible/src/base/nsAccessible.cpp:541
1 nsDocAccessible::FireDocLoadEvents(unsigned int) 	mozilla/accessible/src/base/nsDocAccessible.cpp:779
2 nsDocAccessible::FlushPendingEvents() 	mozilla/accessible/src/base/nsDocAccessible.cpp:1556
3 nsDocAccessible::FlushEventsCallback(nsITimer*, void*) 	mozilla/accessible/src/base/nsDocAccessible.cpp:1619
4 nsTimerImpl::Fire() 	mozilla/xpcom/threads/nsTimerImpl.cpp:400

Comment 1

[Aaron Leventhal]

Attachment: A hunch: fix logic of nsDocAccessible.cpp

I think that RefreshNodes() could happen on the doc accessible, and the children of the doc accessible wouldn't be invalidated even though they should.

It also appears that we do not do any node refreshing when the root of the subtree has no accessible. That's not right.

Comment 2

[Aaron Leventhal]

Also, nsAccessible::Shutdown() is called it does an InvalidateChildren() on the parent. It uses mParent to do this. So if it is possible for the nsDocAccessible to create the children, but for them not to store mParent, then that wouldn't work. I don't know how that's possible, though.

Comment 3

[alexander :surkov (:asurkov)]

the patch looks ok though i don't get why you changed the order of menupopup events firing.

Comment 4

[Aaron Leventhal]

The order of the event firing wasn't why I moved it. I wanted to keep it inside the if block where I know there is an accessible object.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 297665 [details] [diff] [review]
A hunch: fix logic of nsDocAccessible.cpp

ok if event order doesn't make a difference then the patch is ok with me

Comment 6

[Aaron Leventhal]

Surkov, I think you mean r+ not sr+
Maybe I requested it incorrectly? Let's fix that before we seek approval.

Comment 7

[Aaron Leventhal]

Comment on attachment 297665 [details] [diff] [review]
A hunch: fix logic of nsDocAccessible.cpp

Let's change to r=

Comment 8

[Mike Schroepfer]

Comment on attachment 297665 [details] [diff] [review]
A hunch: fix logic of nsDocAccessible.cpp

Now on blocking list - patches don't need approval to land

Comment 9

[Aaron Leventhal]

Marco, can you run with this and make sure it doesn't do anything evil to the build? Here's a try server build:
https://build.mozilla.org/tryserver-builds/2008-01-21_18:[e-mail]-Crash-412878/

Comment 10

[Aaron Leventhal]

Let's hope this fixes the problem ... fix number 1000 for me :)

Comment 11

[Aaron Leventhal]

Marco found a problem with dynamic content updates:

"go to crash-stats.mozilla.com, click Top Crashers, Firefox 3.0b3pre, and then the no. 23 crash. On the page that appears, click the "Reports" link. With JAWS running, the content isn't updated to show the table with crashers."

Comment 12

[Aaron Leventhal]

Attachment: If doc accessible refreshed we must InvalidateChildren if there are any, otherwise we will retain invalid pointers to mFirstChild etc.

Comment 13

[Aaron Leventhal]

Comment on attachment 297665 [details] [diff] [review]
A hunch: fix logic of nsDocAccessible.cpp

The new change is smaller than this one, we need to have Marco test and see if it creates problems.

It should be tested alongside the latest patch in bug 409473.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 298727 [details] [diff] [review]
If doc accessible refreshed we must InvalidateChildren if there are any, otherwise we will retain invalid pointers to mFirstChild etc.

looks good

Comment 15

[Aaron Leventhal]

Comment on attachment 298727 [details] [diff] [review]
If doc accessible refreshed we must InvalidateChildren if there are any, otherwise we will retain invalid pointers to mFirstChild etc.

Checked this in, but leaving the bug open for more related work in RefreshNodes()

Comment 16

[Aaron Leventhal]

Attachment: Follow-up: 1) Correct the 2 cases (there is/isn't an accessible) -- especially we need to still shut down dom subtree's accessible if the root has no accessible 2) Don't reuuse iterNode variable

Comment 17

[Aaron Leventhal]

Attachment: Same thing, diff -uw (no whitespace changes)

Comment 18

[Aaron Leventhal]

Try server builds here:
https://build.mozilla.org/tryserver-builds/2008-01-23_16:[e-mail]-RefreshNodes412878/

Comment 19

[Marco Zehe (:MarcoZ)]

This looks much better! No problems with dynamically updating content, yet I still think that browsing is a bit smoother than before. Good work!

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 298761 [details] [diff] [review]
Follow-up: 1) Correct the 2 cases (there is/isn't an accessible) -- especially we need to still shut down dom subtree's accessible if the root has no accessible 2) Don't reuuse iterNode variable

r=me for the patch -w

Comment 21

[Aaron Leventhal]

Comment on attachment 298761 [details] [diff] [review]
Follow-up: 1) Correct the 2 cases (there is/isn't an accessible) -- especially we need to still shut down dom subtree's accessible if the root has no accessible 2) Don't reuuse iterNode variable

Once again I forgot that we don't need a= for blockers!

Comment 22

[Aaron Leventhal]

We're still getting 1-2 of these each day:
http://crash-stats.mozilla.com/report/list?range_unit=months&query_search=stack&query_type=contains&version=Firefox%3A3.0b3pre%2CSeaMonkey%3A2.0a1pre%2CSunbird%3A0.6a1%2CThunderbird%3A3.0a1pre&signature=nsAccessible%3A%3AInvalidateChildren()&query=FireDocLoadEvents&range_value=1

The crashes started on January 10:

Steve Lee submitted one this morning which is reproduceable for him on Linux, which involves http://www.bloglines.com/public/stevelee
Unfortunately it requires his login to reproduce. 

Some clues:
1. The crash stack occurs when a page load is finished
2. Steve Lee gets it when loading a link into a frame

Perhaps it's due to incorrect cache updating when loading just a frame.

Comment 23

[Aaron Leventhal]

Attachment: Don't fire doc load finished events when doc accessible has been shut down

Don't fire doc load finished events for docs that are already shut down.

Comment 24

[Steve Lee]

(In reply to comment #22)
> Steve Lee submitted one this morning which is reproduceable for him on Linux,

And on another PC also with Ubuntu, but not on Windows.

Comment 25

[Steve Lee]

I tried loads of other frame examples and sites but cannot get it to happen any place else. Still I have a test for the fix.

Comment 26

[alexander :surkov (:asurkov)]

Comment on attachment 301544 [details] [diff] [review]
Don't fire doc load finished events when doc accessible has been shut down

6:37 -0000
>@@ -191,33 +191,45 @@ NS_IMETHODIMP nsAccessibilityService::On
>   GetAccessibleFor(domDocRootNode, getter_AddRefs(accessible));

btw, could you rename domDocRootNode to domDocNode because when I see this name
I think of document element for sure if you're agree root may confuse

>   nsCOMPtr<nsPIAccessibleDocument> docAccessible =
>     do_QueryInterface(accessible);
>   NS_ENSURE_TRUE(docAccessible, NS_ERROR_FAILURE);
> 
>-  PRUint32 eventType = 0;
>+  PRUint32 eventType;

I'd personally prefer to initialize local variables.

>+  if (aStateFlags & STATE_STOP) {
>+    nsCOMPtr<nsIAccessibleDocument> docAccessible =
>+      nsAccessNode::GetDocAccessibleFor(domDocRootNode);

Why do we try to get doc accessible here because we got it above already?

Comment 27

[Aaron Leventhal]

Attachment: Address Surkov's comments

Comment 28

[Aaron Leventhal]

Attachment: Correct patch

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 301660 [details] [diff] [review]
Correct patch

look ok.

>+  if (aStateFlags & STATE_STOP) {
>+    // Do not create accessible for page load end events
>+    // in case it was already shut down before page finished loading
>+    docAccessible = nsAccessNode::GetDocAccessibleFor(domDocNode);  // Cached doc accessibles only

what is idea to use GetDocAccessibleFor(nsIDOMNode*) instead of GetDocAccessibleFor(nsIDocument*) directly?

Comment 30

[Aaron Leventhal]

No reason -- I can switch to the more direct method.

Comment 31

[Steve Lee]

Aaron, did the 50/50 fix go in? If so I'm afraid it still crashes.

http://crash-stats.mozilla.com/report/index/d85c3140-d550-11dc-998f-001a4bd46e84
http://crash-stats.mozilla.com/report/index/ecfa470f-d550-11dc-a644-001a4bd46e84

Comment 32

[Steve Lee]

Hey, hey, hey
OK, so now I've read my email and used the try server version (#2) I cannot make it crash, despite trying like mad.
So it looks like it was 100% the right fix ;-)

Comment 33

[Aaron Leventhal]

Fix checked in. Let's close in a week if the crashes no longer show up on crash-stats.

Comment 34

[Steve Lee]

I still haven't had the crash since using try build.

Comment 35

[Aaron Leventhal]

Ok that does sound good. We can reopen if it appears again.

Comment 36

[Aaron Leventhal]

Crashes are still occuring, maybe not quite as often:
http://crash-stats.mozilla.com/report/index/425e3a8c-dbb6-11dc-ba5e-001a4bd43e5c
http://crash-stats.mozilla.com/report/index/da0c2a80-dbaf-11dc-b914-001a4bd43e5c

and, this last checkin apparently caused bug 417249.

Comment 37

[Aaron Leventhal]

Attachment: Don't do anything w/ doc loads until after a delay

Marco & Scott, try server builds are being built, called
DocLoadFixes_417249_417828_417578

They'll appear here:
https://build.mozilla.org/tryserver-builds/?C=M;O=D

Comment 38

[Aaron Leventhal]

Attachment: Don't do anything w/ doc loads until after a delay

Marco & Scott, try server builds are being built, called
DocLoadFixes_417249_417828_417578

They'll appear here:
https://build.mozilla.org/tryserver-builds/?C=M;O=D

Comment 39

[Aaron Leventhal]

Try builds are here:
https://build.mozilla.org/tryserver-builds/2008-02-15_13:[e-mail]-DocLoadFixes_417249_417828_417578/

Comment 40

[Aaron Leventhal]

Attachment: Don't do anything w/ doc loads until after a delay, and hold onto strong ref of objects we deal with when waiting for timer callback

I believe the strong refs will help with the crashes.

Also, by waiting for a timer we should be sure to have the correct document and create the correct doc accessible. That should help with crashes, as well as make sure we create the doc accessible that we need at start of load.

Comment 41

[Marco Zehe (:MarcoZ)]

(In reply to comment #40)
> -  }
> +  NS_ASSERTION(privDocAccessible, "No nsPIAccessibleDocument for NsIAccessibleDocument");
> +  privDocAccessible->FireDocLoadEvents(eventType);
>  
> -  return NS_OK;
> +
> +void nsAccessibilityService::EndLoadCallback(nsITimer *aTimer, void *aClosure)

This results in a missing } at the end of the StartLoad function, the patch doesn't compile as it is. :-)

Comment 42

[Aaron Leventhal]

I noticed that last night as well. Just put the extra } in to build, or use the pre-made try builds. I was fixing the whitespace last minute. But, the code is still correct other than that.

Comment 43

[Steve Lee]

I've got a little lost here. should I use the above try servers or wait for new ones or a new nightly?

Comment 44

[Aaron Leventhal]

Use the above try server builds to test this patch. This hasn't been reviewed or checked in yet.

Comment 45

[alexander :surkov (:asurkov)]

It sounds you do not use this
nsCOMPtr<nsIWebProgress> mStartLoadProgress, mEndLoadProgress; // Hold strong ref

Comment 46

[alexander :surkov (:asurkov)]

Is it possible to get two notifications (OnStateChange) for two different webprogresse the same time?

Comment 47

[Aaron Leventhal]

Surkov, I'm not sure if 2 are possible at the same time. We should probably handle that. But, it's definitely something that would be hard for me to deal with right now since I'm going away for about 10 days. Would you be willing to work on that?

And, how does everything else look?

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #47)
> Surkov, I'm not sure if 2 are possible at the same time. We should probably
> handle that. But, it's definitely something that would be hard for me to deal
> with right now since I'm going away for about 10 days. Would you be willing to
> work on that?
>

If nobody feels hot on this then I can take this.

> And, how does everything else look?
> 

I didn't accomplish to review the patch actually. But in general it look ok excepting that question.

Comment 49

[Aaron Leventhal]

Attachment: Experiment -- don't fire doc load events for about:blank

Comment 50

[Aaron Leventhal]

Attachment: Corrected experimental patch which doesn't fire about:blank events

Comment 51

[Aaron Leventhal]

Fixed via checkin to bug 417249.