1243267 - Assertion failure: mScrollEvent, at nsGfxScrollFrame.cpp:4274, while scrolling twitter

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Twice in the past few days I've seen this fatal assertion:

Assertion failure: mScrollEvent, at mozilla/layout/generic/nsGfxScrollFrame.cpp:4274

while scrolling down on https://twitter.com/

Comment 1

[Mats Palmgren (inactive)]

Maybe a regression from bug 1209970?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Yeah, probably. Looking at the code I'm not entirely sure why it would happen - although I guess it might if two calls to WillRefresh are scheduled before the first one is dispatched. Then the second call would run into a null mScrollEvent and hit this assertion. I can put up a patch that guards against this.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This should fix the assertion (by getting rid of it) and handling that scenario. If it is the scenario I described above then not sending the second scroll event should be fine because the scroll position would not have changed between the two calls (otherwise mScrollEvent would have been re-created).

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8712742 [details] [diff] [review]
Patch

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Yeah, probably. Looking at the code I'm not entirely sure why it would
> happen - although I guess it might if two calls to WillRefresh are scheduled
> before the first one is dispatched.

What do you mean by "scheduled"?  WillRefresh is called synchronously from
the RefreshDriver here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?rev=3b5f43556647#1659
and PostScrollEvent() only creates one at a time:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=7a7def04840d#4286
so I don't see how we can get a FireScrollEvent() call (i.e. a WillRefresh
callback) with mScrollEvent being null.  Maybe I'm missing something,
can you elaborate on the series of steps that would lead up to this
assertion?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8712742 [details] [diff] [review]
Patch

That's a good point, I guess that scenario can't actually happen. I'll think about this a bit more. :dbaron, if you see it again and can grab a stack trace that would be helpful. I'm wondering if it's some sort of reentrant behavior.

Comment 6

[Bob Clary [:bc] (inactive)]

http://www.bonprix.it/categoria/per-lei-moda-donna-dalla-a-alla-z-pullover-e-felpe/?page=2

Assertion failure: mScrollEvent, at /mozilla/builds/nightly/mozilla/layout/generic/nsGfxScrollFrame.cpp:4401

Aurora/46, Nightly/47 on Linux and Windows. OSX not tested.
http://hg.mozilla.org/mozilla-central/rev/a87d6d52c1fcfffbd6f44537a9671973802cca13

Once you reproduce you need to clear your history and reload to see it again. Let me know if you need a regression range.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Stack to assertion failure

Thanks! I was able to repro on OS X as well. What appears to be happening is that there's a scroll event listener on the page which does a (sync?) XHR request, which spins a nested event loop. That nested event loop fires the refresh driver which then triggers the same scroll event dispatch. So basically yeah, the ScrollEvent::WillRefresh function is getting called reentrantly, hence the problem.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Patch is the same as before, commit message is updated. Given the scenario this happens in I feel like this solution is probably appropriate, unless we want to avoid this reentrancy problem entirely.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Oops, wrong file

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8721739 [details] [diff] [review]
Patch

I think the root of the problem is that the refresh driver takes
a kungFuDeathGrip on the observer before calling WillRefresh():
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?rev=75dfe10ec44a#1688
so nulling out mScrollEvent doesn't delete it.
It seems like the intention was that it should've been the last ref which
would delete it, causing the dtor to remove it from the refresh driver:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=4189a6261ca9#4382

I seems to me that ScrollEvent is meant to be "one shot" so it
feels wrong that we may get more than one call to FireScrollEvent
on the same event.

I suggest we add a RemoveRefreshObserver call at the start of
ScrollEvent::WillRefresh instead, and keep the assertion as is.
The RemoveRefreshObserver in the dtor is then redundant in case
WillRefresh was called, but it's still needed if not.
I don't think it hurts to call it twice though.

What do you think?

Comment 11

[Mats Palmgren (inactive)]

I'm surprised our fuzz testing framework didn't catch this.  It seems like we should try to
enhance it to include the "spinning the event loop from event handlers using sync XHR (or some
other means) and then doing nasty stuff" case.  Or maybe we have that and we were just unlucky
to not hit this assertion yet?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment 10 sounds reasonable, I'll update the patch accordingly.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8721739 [details] [diff] [review]
Patch

Dropping review for now.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Comment 15

[Pulsebot]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8722044 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1209970
[User impact if declined]: probably not much observable, it's an assertion failure which crashes debug builds. In release I think the only problem is that we sometimes fire two scroll events instead of one, in very specific scenarios. However it is a regression in 46 and the patch is low-risk so I figured it's probably ok to uplift.
[Describe test coverage new/current, TreeHerder]: we have tests that exercise the scroll event code but apparently none that exercise this specific re-entrant code path. jruderman is needinfo'd to see if we can fuzz this scenario.
[Risks and why]: pretty low risk patch
[String/UUID change made/needed]: none

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8722044 [details] [diff] [review]
Patch v2

Fix for recent regression, while not really user-facing, still good to fix before we ship it.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3ef2d0b4297c