801659 - clean up caching of DocAccessible on pres shells, and add fast path to nsAccDocManager::GetDocAccessible()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

fwiw I've seen pldhash stuff involving mDocAccessibleCache show up in profiles.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

dholbert for layout stuff, and surkov for a11y stuff.

Comment 2

[alexander :surkov (:asurkov)]

Do we have a bug to remove hash completely?

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> Do we have a bug to remove hash completely?

I don't think so, work should probably happen in several different bugs, and I'm not sure its worth a meta bug.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 671440 [details] [diff] [review]
patch

>+++ b/accessible/src/generic/DocAccessible.cpp
>@@ -631,17 +631,17 @@ DocAccessible::Shutdown()
[SNIP]
>-  mPresShell->SetAccDocument(nullptr);
>+  mPresShell->SetDocAccessible(nullptr);

>+++ b/layout/base/nsPresShell.cpp
>@@ -932,24 +932,24 @@ PresShell::Destroy()
>+    mDocAccessible->Shutdown();
>+    mDocAccessible = nullptr;

Observation: We appear to be clearing nsPresShell::mDocAccessible twice here, at least in some conditions. The PresShell tells the DocAccessible to Shutdown(), which in turn clears the presshell's docAccessible pointer.  Then, when control returns to the PresShell, it (redundantly) clears its mDocAccessible pointer again.

Maybe that's fine -- I haven't looked at all the code-paths Shutdown() could take, and I'm willing to buy that it might complete without hitting that SetDocAccessible(nullptr) line -- and then the pres-shell would want to be absolutely sure it clears its pointer.

In any case: if this is a problem, it's independent of this bug.  Patch here looks fine to me.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Comment on attachment 671440 [details] [diff] [review]
> patch
> 
> >+++ b/accessible/src/generic/DocAccessible.cpp
> >@@ -631,17 +631,17 @@ DocAccessible::Shutdown()
> [SNIP]
> >-  mPresShell->SetAccDocument(nullptr);
> >+  mPresShell->SetDocAccessible(nullptr);
> 
> >+++ b/layout/base/nsPresShell.cpp
> >@@ -932,24 +932,24 @@ PresShell::Destroy()
> >+    mDocAccessible->Shutdown();
> >+    mDocAccessible = nullptr;
> 
> Observation: We appear to be clearing nsPresShell::mDocAccessible twice
> here, at least in some conditions. The PresShell tells the DocAccessible to
> Shutdown(), which in turn clears the presshell's docAccessible pointer. 
> Then, when control returns to the PresShell, it (redundantly) clears its
> mDocAccessible pointer again.
> 
> Maybe that's fine -- I haven't looked at all the code-paths Shutdown() could
> take, and I'm willing to buy that it might complete without hitting that
> SetDocAccessible(nullptr) line -- and then the pres-shell would want to be
> absolutely sure it clears its pointer.
> 
> In any case: if this is a problem, it's independent of this bug.  Patch here
> looks fine to me.

good catch, I still haven't thought deeply about it, but I suspect your rights its redundant, however I suspect that code isn't particularly perf critical and that the pres shell is in cache there so I suspect its not worth worrying about much.  However if I'm wrong about some of that feel free to ask me to look into it.

Comment 6

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eca899430f0c

Comment 7

[Ed Morley [:emorley]]

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