Closed Bug 527466 Opened 11 years ago Closed 11 years ago

(nsAccessibleTreeWalker) Less frame walking, more node walking.

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: davidb, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(2 files)

Spin off from bug 525579.

We need to not hold on to frames in our tree walker state structure because code can execute that might cause the frame to be deallocated before we are truly done with it.

For example we call our accessibility service's GetAccessible to store the accessible in our tree walker state structure. GetAccessible contains a hard set of possible call patch for us to keep an eye on as our code grows. Also, we can't control what users (us) of the tree walker will do during tree walking.

So let's make the collection of frame siblings more atomic in tree walker, and store something safer for use between states.

Note if we can get this right, we should be able to keep our calls to flush layout (see the 192 fix attempt on bug 525579).
Severity: normal → critical
Keywords: crash
Update: we are moved to using nsWeakFrame in bug 525579. I'd like to use this bug as a holding place for discussing alternatives to walking frames as much as we do.
Keywords: crash
Summary: (nsAccessibleTreeWalker) Holding on to frames causes crashers → (nsAccessibleTreeWalker) Less frame walking, more node walking.
This is the original patch that would have fixed bug 525579 by pulling out frame walking but regressed file and video control a11y (of course): https://bug525579.bugzilla.mozilla.org/attachment.cgi?id=410512
Attached patch patchSplinter Review
this patch should make crash described by bug 529442 unreproducible. However it's might be not fix of real problem.
Assignee: bolterbugz → surkov.alexander
Status: NEW → ASSIGNED
Yes, a decent approach (I have something like this in my queue). I can review a patch when you are ready.
Attached patch patch2Splinter Review
let's try (trunk version)
Attachment #413521 - Flags: review?(marco.zehe)
Attachment #413521 - Flags: review?(bolterbugz)
Comment on attachment 413521 [details] [diff] [review]
patch2

Conditional r+ to land on trunk if this somehow fixes our crasher bug 529371 (and passes a11y mochitests) -- with nits:

But we still aren't scooping up all the sibling frames quickly with this patch (I mistakenly thought your first patch was doing that) and I'm not sure how it fixes the crasher but I didn't see anything dangerous.


>+  nsIAccessible getAccessible(in nsIDOMNode aNode, in nsIPresShell aPresShell,
>+                              in nsIWeakReference aWeakShell,
>+                              in nsIFrame aFrameHint, out boolean aIsHidden);

Why not use an nsWeakFrame?  (I guess we can do that later if necessary)


>+  // Update the frame.
>+  nsCOMPtr<nsIPresShell> presShell(do_QueryReferent(mWeakShell));
>+  NS_ASSERTION(presShell, "Huh? No presshell?");
>+  if (!presShell)
>+    return;
>+
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(mState.domNode);
>+  if (content)
>+    mState.frame = presShell->GetRealPrimaryFrameFor(content);
>+  else
>+    presShell->GetRootFrame();

mState.frame = presShell->GetRootFrame();


>   /**
>-   * Change current state so that its frame is changed to next frame.
>-   *
>-   * @param  aTryFirstChild  [in] points whether we should move to child or
>-   *                         sibling frame
>+   * Make treeWalker to traverse by frame tree if necessary.

"Make treewalker traverse..."
Attachment #413521 - Flags: review?(bolterbugz) → review+
(In reply to comment #6)
> (From update of attachment 413521 [details] [diff] [review])
> Conditional r+ to land on trunk if this somehow fixes our crasher bug 529371
> (and passes a11y mochitests) -- with nits:
> 
> But we still aren't scooping up all the sibling frames quickly with this patch
> (I mistakenly thought your first patch was doing that) and I'm not sure how it
> fixes the crasher but I didn't see anything dangerous.

Yes, we don't. We traverse by frames when it's necessary only. In other cases we get them from content. Therefore it should fix the bug 529371 (make it usproducable).

> >+  nsIAccessible getAccessible(in nsIDOMNode aNode, in nsIPresShell aPresShell,
> >+                              in nsIWeakReference aWeakShell,
> >+                              in nsIFrame aFrameHint, out boolean aIsHidden);
> 
> Why not use an nsWeakFrame?  (I guess we can do that later if necessary)

I still think weakFrame is not needed inside of GetAccessible.

> >+    mState.frame = presShell->GetRealPrimaryFrameFor(content);
> >+  else
> >+    presShell->GetRootFrame();
> 
> mState.frame = presShell->GetRootFrame();

thank you for the catch!
trunk try-server build for the second patch (without David's comments).
Comment on attachment 413521 [details] [diff] [review]
patch2

I tried with a local build made with this patch, and here are my observations:
1. Performance hit introduced in bug 525579 is gone when opening Tools/Add-Ons. Note that I also don't get a crash, but I wasn't getting them with local builds anyway.
2. Video still works.
3. File upload still works in both JAWS and NVDA.

r=me based on these.
Attachment #413521 - Flags: review?(marco.zehe) → review+
Comment on attachment 413510 [details] [diff] [review]
patch

Asking approval on 1.9.2 version of the reviewed patch. Crash from the bug 529371 should hide.
Attachment #413510 - Flags: approval1.9.2?
trunk try-server build for the patch with David's comments - https://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-05f7b4a40db3/
The latest try-server build gives the same results as my local build. No crash, fast opening of Tools/Add-Ons, working video and working file control.
Let's test and land the fix for bug 529371 before landing this one.
Note to drivers: we probably want to land this but are not sure until we test with blocker 529371 fixed. Not sure if that means we should clear the approval request here or ask for pre-approval. Note patch2 is the trunk version.
Linux version of trunk tryserver build referenced in comment 11 WFM, too.
No startup crash anymore in normal mode, and no probs with Tools/Addons either in normal or -safe-mode, checking for extension updates, or updating one (latest NTT by Mossop).  Nice work!
OK I like this patch regardless the bad popup frame tree which will be fixed (I hope) on bug 529371.

I filed bug 530081 for further tidying of our tree walker.
Comment on attachment 413521 [details] [diff] [review]
patch2

Marco this is probably why we see a performance improvement:


>-  }
>-  else {
>-    mState.frame = curFrame->GetNextSibling();
>-  }


We don't attempt to get the sibling if the frame doesn't support nsIAnonymousContentCreator. Alexander we might want to add a comment about this since it might be something we need to rethink down the road?
(In reply to comment #17)
> (From update of attachment 413521 [details] [diff] [review])
> Marco this is probably why we see a performance improvement:
> 
> 
> >-  }
> >-  else {
> >-    mState.frame = curFrame->GetNextSibling();
> >-  }
> 

Yes, this is a reason I think. Even we get realPrimaryFrame for every DOM node but it doesn't hit us because we would did this in GetAccessible if we passed wrong or null frame (this happened because frame->GetNextSibling() and GetNextDOMNode() might get unsynchronized).

> We don't attempt to get the sibling if the frame doesn't support
> nsIAnonymousContentCreator. Alexander we might want to add a comment about this
> since it might be something we need to rethink down the road?

I'm not sure it's worth. We could save this for bug 530081 and accompanying bugs (like expose :before/:after).
(In reply to comment #18)
> (In reply to comment #17)
> > We don't attempt to get the sibling if the frame doesn't support
> > nsIAnonymousContentCreator. Alexander we might want to add a comment about this
> > since it might be something we need to rethink down the road?
> 
> I'm not sure it's worth. We could save this for bug 530081 and accompanying
> bugs (like expose :before/:after).

OK as long as the lag before we tackle that bug doesn't allow us to forget :)
Comment on attachment 413510 [details] [diff] [review]
patch

Alexander, I want to more time to consider this patch.
Attachment #413510 - Flags: approval1.9.2?
(In reply to comment #20)
> (From update of attachment 413510 [details] [diff] [review])
> Alexander, I want to more time to consider this patch.

For the branch?
(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 413510 [details] [diff] [review] [details])
> > Alexander, I want to more time to consider this patch.
> 
> For the branch?

Well, for the branch I don't think we need it. For trunk I'm leaning towards landing it but do we ever need to get the sibling if the frame doesn't support nsIAnonymousContentCreator?
(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (From update of attachment 413510 [details] [diff] [review] [details] [details])
> > > Alexander, I want to more time to consider this patch.
> > 
> > For the branch?
> 
> Well, for the branch I don't think we need it. For trunk I'm leaning towards
> landing it but do we ever need to get the sibling if the frame doesn't support
> nsIAnonymousContentCreator?

Not now I think. We should change something when we'll be about to expose generated content.
OK let's take this on trunk when it opens up.
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/1f455848b086
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: cleana11y
Blocks: treea11y
You need to log in before you can comment on or make changes to this bug.