As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 760354 - implement IsInDocument as accessible flag
: implement IsInDocument as accessible flag
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 759875
  Show dependency treegraph
 
Reported: 2012-05-31 21:12 PDT by alexander :surkov
Modified: 2012-06-12 18:33 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (5.36 KB, patch)
2012-06-09 09:00 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (4.87 KB, patch)
2012-06-11 02:00 PDT, Mark Capella [:capella]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch (v3) (5.16 KB, patch)
2012-06-11 08:53 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch (v4) (5.74 KB, patch)
2012-06-11 09:52 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v5) (5.76 KB, patch)
2012-06-11 10:42 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description User image alexander :surkov 2012-05-31 21:12:46 PDT
spun off bug 759875. DocAccessible::IsInDocument returns wrong result for an accessible that was destroyed and new accessible for the same DOM node was created.

1) add eIsNotInDocument flag to Accessible::StateFlags (don't forget to update AccessibleTypes enum)
2) add IsInDocument() method to Accessible
3) remove DocAccessible::IsInDocument
4) make DocAccessible::UncacheChildrenInSubtree to set IsNotInDocument flag on the accessible
Comment 1 User image Mark Capella [:capella] 2012-06-09 09:00:05 PDT
Created attachment 631667 [details] [diff] [review]
Patch (v1)

I followed what I thought the intent was here, but I'm getting test failures in test_stale.html. Did I mess up the code? Or simply expose the problem we're trying to fix?

Ties into original Bug 676267 ... ?
Comment 2 User image alexander :surkov 2012-06-09 09:02:07 PDT
I think because of
inline bool IsInDocument() const { return mFlags & eIsNotInDocument; }
Comment 3 User image alexander :surkov 2012-06-09 09:03:42 PDT
and because of this
DocAccessible::UncacheChildrenInSubtree(Accessible* aRoot)
{
  mFlags |= eIsNotInDocument;
Comment 4 User image Mark Capella [:capella] 2012-06-11 02:00:49 PDT
Created attachment 631831 [details] [diff] [review]
Patch (v2)

Lord, I've got to stop jumping on bugs I don't fully understand :)

This seems to make more sense, and it passes all tests ...
Comment 5 User image alexander :surkov 2012-06-11 05:55:23 PDT
Comment on attachment 631831 [details] [diff] [review]
Patch (v2)

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

::: accessible/src/generic/Accessible.h
@@ +479,5 @@
>      return mContent->IsHTML() &&
>        (mContent->Tag() == nsGkAtoms::abbr || mContent->Tag() == nsGkAtoms::acronym);
>    }
>  
> +  inline bool IsInDocument(Accessible* aAccessible) const

no inline, I'd keep it near of IsDefunct, please comment it

@@ +484,5 @@
> +  {
> +    Accessible* acc = aAccessible;
> +    while (acc && !acc->IsPrimaryForNode())
> +      acc = acc->Parent();
> +

you don't need that because you set the flag on each accessible you uncache from document

@@ +488,5 @@
> +
> +    return acc ? !(acc->mFlags & eIsNotInDocument) : false;
> +  }
> +
> +  inline void SetIsNotInDocument() { mFlags |= eIsNotInDocument; }

not sure I like it's public (you could friend classes) but perhaps it's ok.
Comment 6 User image Mark Capella [:capella] 2012-06-11 08:53:21 PDT
Created attachment 631904 [details] [diff] [review]
Patch (v3)

I went ahead and tweaked those bits while waiting ...
Comment 7 User image alexander :surkov 2012-06-11 08:56:23 PDT
Comment on attachment 631904 [details] [diff] [review]
Patch (v3)

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

::: accessible/src/generic/Accessible.h
@@ +694,5 @@
> +   */
> +  bool IsInDocument(Accessible* aAccessible) const
> +  {
> +    return aAccessible ? !(aAccessible->mFlags & eIsNotInDocument) : false;
> +  }

you're on accessible so you don't need aAccessible argument, jsut
return !(mFlags & eIsNotInDocument);
Comment 8 User image Mark Capella [:capella] 2012-06-11 08:58:35 PDT
I though I tried that and it was failinng ... lemme try again ...
Comment 9 User image Trevor Saunders (:tbsaunde) 2012-06-11 09:16:10 PDT
Comment on attachment 631904 [details] [diff] [review]
Patch (v3)

># HG changeset patch
># Parent 0f6ae110182a267ab1a20bca6730194cbff72ea3
># User Mark Capella <markcapella@twcny.rr.com>
>Bug 760354 - implement IsInDocument as accessible flag, r=surkov, f=tbsaunde

might want to update that ;-)

>+  bool IsInDocument(Accessible* aAccessible) const
>+  {
>+    return aAccessible ? !(aAccessible->mFlags & eIsNotInDocument) : false;

if we're going to move this from DocAccessible to Accessible we should get rid of the argument and make it work on this, and then update the callers.  Hopefully that'll help perf some since callers won't need to get the doc anymore.
Comment 10 User image Mark Capella [:capella] 2012-06-11 09:52:42 PDT
Created attachment 631927 [details] [diff] [review]
Patch (v4)

This is how I had the patch without passing an argument ... it fails test_stale.html with:

failed | wrong extra state bits for ['div node', address: [object HTMLDivElement], role: section, address: [xpconnect wrapped nsIAccessible]]!got '0', expected 'stale'

Changing it to take the arg works / passes ... (I had the debugger out and was hacking at it for awhile before I tried the new approach).
Comment 11 User image Mark Capella [:capella] 2012-06-11 10:42:03 PDT
Created attachment 631942 [details] [diff] [review]
Patch (v5)

Changed the caller and this works again ...
Comment 12 User image Mark Capella [:capella] 2012-06-11 17:05:49 PDT
push to inbound TRY
https://tbpl.mozilla.org/?tree=Try&rev=314aed2afe92
Comment 13 User image Mark Capella [:capella] 2012-06-12 02:50:04 PDT
previous push was in the middle of many TRY and INBOUND issues ... lotsa backouts and such ... waited, and am trying again:

https://tbpl.mozilla.org/?tree=Try&rev=982d37b69ab6
Comment 14 User image Mark Capella [:capella] 2012-06-12 08:27:15 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ab582792079c
Comment 15 User image Matt Brubeck (:mbrubeck) 2012-06-12 18:33:23 PDT
https://hg.mozilla.org/mozilla-central/rev/ab582792079c

Note You need to log in before you can comment on or make changes to this bug.