1243583 - Assertion failure: IsIdle(oldState), at xpcom/glue/PLDHashTable.h:140

Status: VERIFIED FIXED

(Core :: DOM: Animation, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: backtrace of assertion failure

STR:
 1. Load attached testcase (coming up) in a debug build.

(The testcase has an iframe that has an event handler which removes the iframe from its parent document.)

ACTUAL RESULTS:
Assertion failure: IsIdle(oldState), at /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/xpcom/glue/PLDHashTable.h:140


The backtrace (attached) shows this is happening in EventListener::HandleEvent calling into JS, unsurprisingly.

Marking this as security-sensitive because:
 (1) even though we're not crashing, this seems like we might be on the edge of using freed memory.
 (2) I hit this when trying to come up with a testcase for bug 1243555 (and this might be close to something usable as an attack-vector for that bug), so we probably shouldn't open this up until bug 1243555 is no longer a threat.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: (iframe for use in testcase 1)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (asserts fatally in debug builds)

Comment 3

[Daniel Holbert [:dholbert]]

<tangent>
With a slightly-different testcase (using a click handler instead of a transitionend handler), I instead triggered this nonfatal assertion (without any crash/abort):
###!!! ASSERTION: Null pres shell: 'mShell', file $OBJ/dist/include/nsPresContext.h, line 171

That's inside the "PresShell()" accessor (which, by the lack of a "Get" in its name, promises to return something non-null).  So, we're failing to live up to that promise, with my other testcase.
</tangent>

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (asserts non-fatally in debug builds)

Here's the testcase I'm talking about in comment 3 (which asserts non-fatally about "Null pres shell")

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Not really an events bug, but something else.
I wonder if this is a regression.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Ah, this is an issue in animation code.
DispatchAnimationEventsOnSubDocuments is scary. enumerates through the child documents and while doing that dispatches events.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

So a regression form bug 1180125 I think.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

And the fix is possible to collected first all the relevant documents to an array which keeps strong references to the documents, and then dispatch events.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8712952 [details]
testcase 2 (asserts non-fatally in debug builds)

(Thanks for the diagnosis, smaug!  I spun off testcase 2 into a new bug -- bug 1243628 -- since it doesn't involve DispatchAnimationEventsOnSubDocuments & hence seems to need a different fix.)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I guess I could take this. Looks rather obvious case.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I went through other cases where we enumerate subdocuments and couldn't see other scary cases.
But we should make that API less error prone.

Comment 13

[Daniel Holbert [:dholbert]]

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

Review of attachment 8713741 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with 'i' nit addressed.

::: layout/base/nsRefreshDriver.cpp
@@ +1486,5 @@
>  
> +  nsCOMArray<nsIDocument> documents;
> +  CollectDocuments(mPresContext->Document(), &documents);
> +
> +  for (int32_t i = 0; i < documents.Length(); ++i) {

nsCOMArray::Length() returns an uint32_t value, so 'i' should have type uint32_t as well. (Otherwise I expect this will trigger a signed/unsigned comparison compiler warning.)

@@ +1506,5 @@
> +    // below animations in terms of compositing order.
> +    context->TransitionManager()->DispatchEvents();
> +    // Check that the presshell has not been destroyed
> +    if (context->GetPresShell()) {
> +      context->AnimationManager()->DispatchEvents();

I'm confused by the GetPresShell() check here, but it's orthogonal to this change (it exists in the old & the new code, and is partially-explained in bug 1211334).  I'll spin off a separate bug for my concerns/confusion about this chunk.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

oh right, Count() returns int32_t, Length() uint32_t. /me kicks the API.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: uint32_t

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: int32_t everywhere

Just use the saner old style nsCOMArray API.

Comment 17

[Daniel Holbert [:dholbert]]

That works too. Thanks.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8713758 [details] [diff] [review]
int32_t everywhere

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
At least crash is relatively easily constructible.

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


Which older supported branches are affected by this flaw?
44, 45, 46

If not all supported branches, which bug introduced the flaw?
bug 1180125

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch seems to apply cleanly to beta, and fixes the bug there, so the same patch should work everywhere.

How likely is this patch to cause regressions; how much testing does it need?
Should be safe. We just collect documents to an array before doing anything with those documents.

Bug 1244235 even adds a new testcase related to this code.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

Forgot to fill this. 
The commit message could be
"Bug 1243583, ensure transition events are dispatched to all the relevant subdocuments, r=dholbert"

Comment 20

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

sec-approval+ for checkin on February 9. 

Can we get branch nominations to have it go onto branches after trunk?

Comment 21

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

Comment on attachment 8713758 [details] [diff] [review]
int32_t everywhere

Nevermind. I see the nominations now. Approving.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

We probably want to land Bug 1244250 at the same time, since it is dealing with same stuff as this
(dispatching transition/animation events).

Comment 23

[Daniel Holbert [:dholbert]]

(Canceling needinfo for birtles, since smaug ended up taking this [thanks!])

Comment 24

[Carsten Book [:Tomcat]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/2d13bd207e3d

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d13bd207e3d

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba2f607e26a8

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd4dc4e822d6

Comment 28

[Petruta Horea [:phorea], Desktop QA]

Reproduced the crash and the assertion failure using Nightly debug build 47.0a1 2016-01-27 under Win 7 64-bit.

Verified as fixed using latest Nightly 47.0a1 and Firefox 45.0 2016-03-03.

Comment 29

[Intermittent Failures Robot]

1 failures in 2581 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform breakdown:

• windows7-32: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1243583&startday=2019-06-17&endday=2019-06-23&tree=all

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.