1243628 - ###!!! ASSERTION: Null pres shell: 'mShell', file ../../dist/include/nsPresContext.h, line 171

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: (iframe for use in testcase)

STR:
 1. Load attached testcase in a debug build.
 2. Click the "Click me" div.

ACTUAL RESULTS:
###!!! ASSERTION: Null pres shell: 'mShell', file ../../dist/include/nsPresContext.h, line 171

(This is inside the "PresShell()" method, which promises to return non-null [based on the lack of "Get" in its name], and which is asserting because it's about to break that promise.)

EXPECTED RESULTS:
No assertion.


(I originally posted a version of this testcase on bug 1243583, but I'm spinning it off into its own bug since its backtrace looks sufficiently different.)

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> (This is inside the "PresShell()" method, which promises to return non-null
> [based on the lack of "Get" in its name], and which is asserting because
> it's about to break that promise.)

Fortunately we don't actually dereference this null pointer & crash, because we null-check the result of PresShell in the caller for some reason (even though we shouldn't have to):
> 935   if (aPresContext->PresShell()) {
> 936     pt = pt.RemoveResolution(nsLayoutUtils::GetCurrentAPZResolutionScale(aPresContext->PresShell()));
> 937   }
http://mxr.mozilla.org/mozilla-central/source/dom/events/Event.cpp?rev=b9f5b9c3d5a1#935

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: backtrace of assertion-failure (with e10s)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: backtrace of assertion-failure (with e10s disabled)

Attaching backtraces with & without e10s, for an abundance of thoroughness.  (They're the same up until stack level 17 -- nsWindow::DispatchEvent vs PuppetWidget::DispatchEvent, just how we dispatch the click event.)

Bottom line, this reproduces regardless of whether e10s is enabled.

Comment 5

[Daniel Holbert [:dholbert]]

The nsPresContext in question (whcih we're calling PresShell on) is still alive, happily.  The Event keeps it alive;
> 260   RefPtr<nsPresContext>     mPresContext;
http://mxr.mozilla.org/mozilla-central/source/dom/events/Event.h?rev=85e218929a7a#260

Nonetheless, we've already called PresShell::Destroy() at this point (when the iframe gets removed from the document), which includes a call to mPresContext->SetShell(nullptr).  This is why our later nsPresContext::PresShell() call finds itself with a null pointer.

Comment 6

[Daniel Holbert [:dholbert]]

Seems likely that the most proximal cause of this assertion-failure is Bug 1224015, which added this call to PresShell() at stack-level 2 in the backtrace -- the call that asserts.

But it seems bad that our Event's mPresContext has been detached from its PresShell, before we've finished with it.  Seems like somewhere in the backtrace, we should have a kungfudeathgrip of some sort to keep any affected PresShells alive until after event-handling has been completed.  Or maybe the Event itself should have a refcounted pointer to the PresShell?

Comment 8

[Timothy Nikkel (:tnikkel)]

The presshell nulls out the prescontext's pointer to itself in PresShell::Destroy, which is called in various function in document viewer, which don't seem to be directly tied to ref count (I didn't dig too deep). So even if we hold a reference to the presshell that doesn't obviously stop someone from calling Destroy on the presshell.

Perhaps we should just change the call to nsPresContext::GetPresShell (which doesn't assert) instead of nsPresContext::PresShell?

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: patch

The rest of Event.cpp uses GetPresShell (vs PresShell), so seems like we are on the right track.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8713029 [details] [diff] [review]
patch

Ah, good point.

All these "GetPresShell" calls (notably not "PresShell") -- combined with the strong nsPresContext reference -- must mean this detached-from-shell situation is a normal-enough occurrence.  So, my concerns at the end of comment 6 (about needing to handle this more robustly) are probably unfounded.

r=me

Comment 11

[Daniel Holbert [:dholbert]]

I think we should be OK to open this bug up and land its patch, since there's nothing that's really security-sensitive here, aside from connections to other bugs.

So, I'm going to mark a few comments & the testcase as 'private' to avoid giving away those connections & the makings of a testcase for one of the worse bugs.

Comment 13

[Daniel Holbert [:dholbert]]

dveditz (or anyone with privileges to do so), would you mind opening this bug up? I don't have privileges to remove the 'security-sensitive' flag.

Alternately, if you think we should err on the side of keeping this hidden, I guess we can go through the normal security-approval landing process here (though I don't think it's worth bothering with backports).

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8713029 [details] [diff] [review]
patch

Looks like needinfo is taking awhile. We should just land this. Guess sec-approval it is.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
the patch does not address a security issue

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no sec problem

Which older supported branches are affected by this flaw?
aurora maybe beta? I didn't check

If not all supported branches, which bug introduced the flaw?
it's mentioned in the comments here somewhere

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
trivial

How likely is this patch to cause regressions; how much testing does it need?
safe patch

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Timothy Nikkel (:tnikkel) from comment #14)
> [Security approval request comment]
> Which older supported branches are affected by this flaw?
> aurora maybe beta? I didn't check
> 
> If not all supported branches, which bug introduced the flaw?
> it's mentioned in the comments here somewhere

Yeah, comment 6 -- this was introduced in bug 1224015, which means versions 45 and newer are affected (but no need to backport; the patch doesn't change behavior in opt builds at all. This bug's patch is effectively just making us bypass an assertion that we can't & don't need to depend on.)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8713029 [details] [diff] [review]
patch

tn, looks like you're good to land.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8713029 [details] [diff] [review]
patch

No sec-approval+ needed so clearing.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cea9bfd3244

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cea9bfd3244