801808 - DocAccessible should probably assume & assert that it has a non-null pres shell

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Daniel Holbert [:dholbert]]

While reviewing bug 801659, I noticed that the DocAccessible constructor seems to think it's possible that it might receive a null presshell:
>   if (mPresShell)
>     mPresShell->SetAccDocument(this);
https://mxr.mozilla.org/mozilla-central/source/accessible/src/generic/DocAccessible.cpp#88

...but the code that invokes it, nsAccDocManager::CreateDocOrRootAccessible(), explicitly bails out if it's got no pres shell:
>    // Ignore documents without presshell and not having root frame.
>    nsIPresShell* presShell = aDocument->GetShell();
>    if (!presShell || !presShell->GetRootFrame() || presShell->IsDestroying())
>      return nullptr;
https://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccDocManager.cpp#348

So if I'm understanding correctly, I think DocAccessible should boldly assume (and assert) that it's got a non-null mPresShell.

Comment 1

[Trevor Saunders (:tbsaunde)]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> While reviewing bug 801659, I noticed that the DocAccessible constructor
> seems to think it's possible that it might receive a null presshell:
> >   if (mPresShell)
> >     mPresShell->SetAccDocument(this);
> https://mxr.mozilla.org/mozilla-central/source/accessible/src/generic/
> DocAccessible.cpp#88
> 
> ...but the code that invokes it,
> nsAccDocManager::CreateDocOrRootAccessible(), explicitly bails out if it's
> got no pres shell:
> >    // Ignore documents without presshell and not having root frame.
> >    nsIPresShell* presShell = aDocument->GetShell();
> >    if (!presShell || !presShell->GetRootFrame() || presShell->IsDestroying())
> >      return nullptr;
> https://mxr.mozilla.org/mozilla-central/source/accessible/src/base/
> nsAccDocManager.cpp#348
> 
> So if I'm understanding correctly, I think DocAccessible should boldly
> assume (and assert) that it's got a non-null mPresShell.

sounds good, note this wasn't actually true until bug 741408 was fixed, because we used to have some crazy code on linux that made documents without pres shells.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 3

[Daniel Holbert [:dholbert]]

I think this is all true of "mDocument" as well (we null-check it for no reason), but I'll leave that for another bug since there are more of those checks sprinkled around throughout the DocAccessible impl.  (Those null-checks make sense, given what Trevor says in comment 1)

Trevor: since this touches the same code as bug 801659, I'll hold off on landing this until you've landed bug 801659, so that I won't bitrot you there.

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 671567 [details] [diff] [review]
fix

>+  NS_ABORT_IF_FALSE(mPresShell, "should have been given a pres shell");
>+  mPresShell->SetAccDocument(this);

this looks right, and btw yes, bug 741408 should let us remove all the mDocument checks.

I might use MOZ_ASSERT / NS_ASSERTION, but we're going to crash here the purpose of the assert is just to make it clear why, so whatever.

thanks for the patch.

Comment 5

[Daniel Holbert [:dholbert]]

Cool, I'll switch to MOZ_ASSERT.  Thanks for the review!

Comment 6

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c118efe23e9e

Comment 7

[Ed Morley [:emorley]]

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