1443518 - Crash in mozilla::layers::InputBlockState::SetConfirmedTargetApzc

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-1bd0164f-4a34-40ef-8c25-7c6110180306.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::layers::InputBlockState::SetConfirmedTargetApzc gfx/layers/apz/src/InputBlockState.cpp:70
1 xul.dll mozilla::layers::InputQueue::ConfirmDragBlock gfx/layers/apz/src/InputQueue.cpp:721
2 xul.dll mozilla::layers::APZCTreeManager::StartScrollbarDrag gfx/layers/apz/src/APZCTreeManager.cpp:758
3 xul.dll mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::layers::IAPZCTreeManager>, void  xpcom/threads/nsThreadUtils.h:1200
4 xul.dll mozilla::layers::APZThreadUtils::RunOnControllerThread gfx/layers/apz/util/APZThreadUtils.cpp:65
5 xul.dll mozilla::layers::APZCTreeManagerParent::RecvStartScrollbarDrag gfx/layers/ipc/APZCTreeManagerParent.cpp:279
6 xul.dll mozilla::layers::PAPZCTreeManagerParent::OnMessageReceived ipc/ipdl/PAPZCTreeManagerParent.cpp:448
7 xul.dll mozilla::layers::PCompositorManagerParent::OnMessageReceived ipc/ipdl/PCompositorManagerParent.cpp:121
8 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2133
9 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2063

=============================================================

There are 3 crashes (from 1 installation) in nightly 60 with buildid 20180306100123. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1437694.

[1] https://hg.mozilla.org/mozilla-central/rev?node=c0b2aa2dc564

Comment 1

[Botond Ballo [:botond]]

Yes, this is definitely caused by bug 1437694. The diagnostic assert firing indicates an underlying bug in compositor hit testing.

If people who are experiencing this crash could provide STR (in particular, what page the crash occurred on), that would be very helpful.

Comment 2

[Bob Clary [:bc] (inactive)]

STR

1. https://www.marketwatch.com/story/now-the-bad-news-and-slightly-less-horrible-news-about-saving-for-retirement-2018-03-07

2. Select investment style select and attempt to scroll the choices. Reproduced with Today's Nightly on macos.

bp-8ee7d37f-99e8-4384-964e-fee0a0180307

Comment 3

[Botond Ballo [:botond]]

(In reply to Bob Clary [:bc:] from comment #2)
> 2. Select investment style select and attempt to scroll the choices.

Hmm, I can't find that on the page. Any chance you could post a screenshot?

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: 1443518.png

Just above Get Your Score, the select begins with Growth with I...

Comment 5

[Bob Clary [:bc] (inactive)]

bp-89bb8795-29d5-4fba-8325-88acd0180307
Linux x86_64

Comment 6

[Botond Ballo [:botond]]

Attachment: bug1443518-1.png

Interesting, I get a different widget there that doesn't have such a dropdown (the "Annually" dropdown is not scrollable).

Comment 7

[Bob Clary [:bc] (inactive)]

You are in Toronto which may be related to the difference. Can you proxy through Mountain View?

Comment 8

[Botond Ballo [:botond]]

Attachment: Reduced testcase

Meanwhile, I was given another URL privately, and reduced it to the attached testcase.

Comment 9

[Botond Ballo [:botond]]

There's a layer with an empty visible region but a nonempty hit region that's on top of the scroll frame and eating the events.

Comment 10

[Botond Ballo [:botond]]

Attachment: Display list dump with LayerEventRegions (layout.simple-event-region-items=false)

Comment 11

[Botond Ballo [:botond]]

Attachment: Display list dump with LayerEventRegions (layout.simple-event-region-items=false)

Comment 12

[Botond Ballo [:botond]]

Attachment: Display list dump with CompositorHitTestInfo (layout.simple-event-region-items=true)

Comment 13

[Botond Ballo [:botond]]

Miko, this appears to be a regression from bug 1434243.

I got display list dumps with CompositorHitTestInfo items (the default) and LayerEventRegions items (with layout.simple-event-region-items=false), attached. In the CompositorHitTestInfo dump, there is a CompositorHitTestInfo item (p=0x7fad4de281a0) in front of the other items which creates an invisible layer (0x7fad4de9f400) that eats the events. There is no such LayerEventRegions item in the LayerEventRegions dump.

Coudl you have a look?

Comment 14

[Miko Mynttinen]

The new CompositorHitTestInfo item is created because the CompositorHitTestInfo (p=0x7fad4de287a0) above has an empty area, and fails the check at [1]. With LayerEventRegions items, the child item was merged with the empty parent item. I need to investigate more to figure out what the expected behavior here is.

[1]: https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/layout/painting/nsDisplayList.cpp#2287

Comment 15

[Miko Mynttinen]

Attachment: Bug 1443518 - Fix incorrect CompositorHitTestInfo ordering for inline lines

Review commit: https://reviewboard.mozilla.org/r/226432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226432/

Comment 16

[Miko Mynttinen]

Since this ended up being a bug with display list item ordering, I've moved this to the appropriate component.

The problem was that the CompositorHitTestInfo item was placed in the child content list, because the line box was inline[1]. This caused it to get incorrectly positioned above the floats.

[1]: https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/layout/generic/nsBlockFrame.cpp#6642

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8957556 [details]
Bug 1443518 - Fix incorrect CompositorHitTestInfo ordering for inline lines

https://reviewboard.mozilla.org/r/226432/#review232626

Does the same bug not also exist for LayerEventRegions? The conditions to trigger it are probably different (since we combine frames under different conditions), but I think the underlying logic bug is still there.

I realise we're phasing that out, but it might be worth moving both while we're at it.

Comment 18

[Miko Mynttinen]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Comment on attachment 8957556 [details]
> Bug 1443518 - Fix incorrect CompositorHitTestInfo ordering for inline lines
> 
> https://reviewboard.mozilla.org/r/226432/#review232626
> 
> Does the same bug not also exist for LayerEventRegions? The conditions to
> trigger it are probably different (since we combine frames under different
> conditions), but I think the underlying logic bug is still there.
> 
> I realise we're phasing that out, but it might be worth moving both while
> we're at it.

I think it does exist for LayerEventRegions as well, but as you mentioned, combining frames avoids this particular problem.

My opinion would be to leave this behavior as it is for LayerEventRegions. This way we could preserve this more mature codepath for reference if we get any more regressions like this. Even if we decide to fix this, we would get almost no testing for this change, as nsDisplayCompositorEventRegion items are used by default.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/271e8d061124
Fix incorrect CompositorHitTestInfo ordering for inline lines r=mattwoodrow

Comment 20

[Botond Ballo [:botond]]

Thank you for fixing this, Miko!

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/271e8d061124

Comment 22

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance.

Comment 23

[Miko Mynttinen]

Comment on attachment 8957556 [details]
Bug 1443518 - Fix incorrect CompositorHitTestInfo ordering for inline lines

Approval Request Comment
[Feature/Bug causing the regression]: An assertion added in bug 1437694 uncovered an existing problem with CompositorHitTestInfo display item ordering.
[User impact if declined]: Possible crashes in Dev edition and Nightly due to MOZ_DIAGNOSTIC_ASSERT. In other versions possibly buggy scrollbar behavior.
[Is this code covered by automated tests?]: No. As this feature requires dragging the scrollbar, implementing automated test is complicated.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes.
STR:
1) Open the attached "Reduced testcase" (https://bug1443518.bmoattachments.org/attachment.cgi?id=8957026)
2) Drag the vertical scrollbar under the text "A line of text that overflows..." back and forth a couple of times.
Expected results: No crash and scrolling the text works.

[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The problem and the fix are well understood and simple. The crash rates for this signature have significantly dropped since this patch was introduced.
[String changes made/needed]: None.

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8957556 [details]
Bug 1443518 - Fix incorrect CompositorHitTestInfo ordering for inline lines

scrollbar crash fix, beta60+

Comment 25

[Eliza Balazs [:ebalazs_]]

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

Comment 26

[[:philipp]]

the patch seems to have somewhat reduced the volume of the crash, but the crash signature is still around in current builds. should we reopen this bug or file a new one about the remaining crashes?

Comment 27

[Botond Ballo [:botond]]

Please file a new bug. Additional URLs that triggered this crash have been provided to me privately, and I plan to test them and reduce them as appropriate.

Thanks!

Comment 28

[Ciprian Georgiu, Desktop QA]

Managed to reproduce this crash using the STR from comment 23, on an affected Nightly build (60.0a1, 2018-03-06)
- https://crash-stats.mozilla.com/report/index/d3cdd22b-c99b-4dd3-b118-e5a520180320

I cannot reproduce it anymore on latest Nightly 61.01 (2018-03-20) and Beta 60.0b5 (20180319175655) on the following OSes: Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.

Since the crash signature is fixed on my side and the remaining crashes will be addressed in bug 1447131, I will close this issue as verified fixed.

Comment 29

[Botond Ballo [:botond]]

(In reply to Miko Mynttinen [:miko] from comment #23)
> [Is this code covered by automated tests?]: No. As this feature requires
> dragging the scrollbar, implementing automated test is complicated.

We've actually recently built out infrastructure to test scrollbar dragging (or at least, hit testing over scrollbars) in APZ mochitests. I'll write one in bug 1449759.