Closed Bug 801659 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

fwiw I've seen pldhash stuff involving mDocAccessibleCache show up in profiles.
Attached patch patchSplinter Review
dholbert for layout stuff, and surkov for a11y stuff.
Attachment #671440 - Flags: review?(surkov.alexander)
Attachment #671440 - Flags: review?(dholbert)
Attachment #671440 - Flags: review?(surkov.alexander) → review+
Do we have a bug to remove hash completely?
(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+
(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.
https://hg.mozilla.org/mozilla-central/rev/eca899430f0c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.