Closed Bug 801808 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

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.
Attached 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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.