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

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
(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.
Posted patch fixSplinter Review
Attachment #671567 - Flags: review?(trev.saunders)
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Depends on: 801659
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.
Attachment #671567 - Flags: review?(trev.saunders) → review+
Cool, I'll switch to MOZ_ASSERT.  Thanks for the review!
Depends on: 741408
https://hg.mozilla.org/mozilla-central/rev/c118efe23e9e
Status: ASSIGNED → 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.