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

RESOLVED FIXED in mozilla19

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug)

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
fwiw I've seen pldhash stuff involving mDocAccessibleCache show up in profiles.
(Assignee)

Comment 1

5 years ago
Created attachment 671440 [details] [diff] [review]
patch

dholbert for layout stuff, and surkov for a11y stuff.
Attachment #671440 - Flags: review?(surkov.alexander)
Attachment #671440 - Flags: review?(dholbert)

Updated

5 years ago
Attachment #671440 - Flags: review?(surkov.alexander) → review+

Comment 2

5 years ago
Do we have a bug to remove hash completely?
(Assignee)

Comment 3

5 years ago
(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 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.
Attachment #671440 - Flags: review?(dholbert) → review+
Blocks: 801808
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/eca899430f0c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.