1457590 - Intermittent gfx/layers/apz/test/mochitest/test_group_mouseevents.html | application crashed [@ mozilla::layers::AsyncPanZoomController::ConvertScrollbarPoint(mozilla::gfx::PointTyped<mozilla::ParentLayer after Assertion failure: mIsSome

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: ncsoregi [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=176001225&repo=autoland

https://queue.taskcluster.net/v1/task/C4TlOa9sS3qVQWi8jiXL4g/runs/0/artifacts/public/logs/live_backing.log

12:05:26     INFO - TEST-PASS | gfx/layers/apz/test/mochitest/test_group_mouseevents.html | helper_drag_scroll.html | Scroll position strictly increased 
12:05:26     INFO - Buffered messages logged at 12:04:37
12:05:26     INFO - must wait for load
12:05:26     INFO - Buffered messages finished
12:05:26    ERROR - TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_mouseevents.html | application terminated with exit code 1
12:05:26     INFO - runtests.py | Application ran for: 0:01:26.715013
12:05:26     INFO - zombiecheck | Reading PID log: /var/folders/_2/5rcdryb97wl3b_q4535l0xqc00000w/T/tmp17ZKFJpidlog
12:05:26     INFO - ==> process 2146 launched child process 2147
12:05:26     INFO - ==> process 2146 launched child process 2148
12:05:26     INFO - ==> process 2146 launched child process 2149
12:05:26     INFO - zombiecheck | Checking for orphan process with PID: 2147
12:05:26     INFO - zombiecheck | Checking for orphan process with PID: 2148
12:05:26     INFO - zombiecheck | Checking for orphan process with PID: 2149
12:05:26     INFO - mozcrash Copy/paste: /Users/cltbld/tasks/task_1524854160/build/macosx64-minidump_stackwalk /var/folders/_2/5rcdryb97wl3b_q4535l0xqc00000w/T/tmpovTx_6.mozrunner/minidumps/1D86CA9E-BCFC-474D-AFAF-CFEC25C082E0.dmp /Users/cltbld/tasks/task_1524854160/build/symbols
12:05:46     INFO - mozcrash Saved minidump as /Users/cltbld/tasks/task_1524854160/build/blobber_upload_dir/1D86CA9E-BCFC-474D-AFAF-CFEC25C082E0.dmp
12:05:46     INFO - mozcrash Saved app info as /Users/cltbld/tasks/task_1524854160/build/blobber_upload_dir/1D86CA9E-BCFC-474D-AFAF-CFEC25C082E0.extra
12:05:46     INFO - PROCESS-CRASH | gfx/layers/apz/test/mochitest/test_group_mouseevents.html | application crashed [@ mozilla::layers::AsyncPanZoomController::ConvertScrollbarPoint(mozilla::gfx::PointTyped<mozilla::ParentLayerPixel, float> const&, mozilla::layers::ScrollbarData const&) const]
12:05:46     INFO - Crash dump filename: /var/folders/_2/5rcdryb97wl3b_q4535l0xqc00000w/T/tmpovTx_6.mozrunner/minidumps/1D86CA9E-BCFC-474D-AFAF-CFEC25C082E0.dmp
12:05:46     INFO - Operating system: Mac OS X
12:05:46     INFO -                   10.10.5 14F27
12:05:46     INFO - CPU: amd64
12:05:46     INFO -      family 6 model 69 stepping 1
12:05:46     INFO -      4 CPUs
12:05:46     INFO - 
12:05:46     INFO - GPU: UNKNOWN
12:05:46     INFO - 
12:05:46     INFO - Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
12:05:46     INFO - Crash address: 0x0
12:05:46     INFO - Process uptime: 37 seconds
12:05:46     INFO -

Comment 1

[Intermittent Failures Robot]

2 failures in 780 pushes (0.003 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 1
* autoland: 1

Platform breakdown:
* osx-10-10: 1
* linux64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1457590&startday=2018-04-23&endday=2018-04-29&tree=trunk

Comment 3

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

According to Ben's comment in bug 1458400 comment 0, it's the *thumbData.mDirection at [1] that's asserting. Which is odd, because in the caller function we assert [2]. And these are both MOZ_ASSERTs so it's not a debug vs opt build issue. Somehow mDirection is getting set to Nothing in between [1] and [2]? Maybe a race condition with the updater thread updating the hit-testing tree node while this is running on the controller thread without holding the tree lock?

[1] https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/gfx/layers/apz/src/AsyncPanZoomController.cpp#1754
[2] https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/gfx/layers/apz/src/APZCTreeManager.cpp#1782

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Note, I only think its the mDirection because thats the only Maybe<> I could find involved in the code.  It seems like a lot of inlining was happening, though, which made it hard to be completely sure.

Comment 5

[Intermittent Failures Robot]

2 failures in 551 pushes (0.004 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* try: 1
* mozilla-inbound: 1

Platform breakdown:
* linux64: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1457590&startday=2018-04-30&endday=2018-05-06&tree=trunk

Comment 7

[Intermittent Failures Robot]

2 failures in 660 pushes (0.003 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-central: 2

Platform breakdown:
* macosx64-qr: 1
* linux64-ccov: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1457590&startday=2018-05-07&endday=2018-05-13&tree=trunk

Comment 8

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

I think comment 3 is accurate. We can fix either by excluding scrollbar nodes from being recycled, or ensure that we hold the tree lock while handling input events that fiddle with the scrollbar nodes. The latter is probably more robust, but we'll need to make sure we don't introduce any lock ordering violations.

Comment 10

[Intermittent Failures Robot]

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

Repository breakdown:
* autoland: 1

Platform breakdown:
* linux64-qr: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1457590&startday=2018-05-14&endday=2018-05-20&tree=trunk

Comment 11

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

I discussed this with Botond today, and we decided to do the following to solve the ordering violation:
- Add a flag on the HTTN that tracks if the node can be recycled. This flag would only be read/mutated while holding the APZ tree lock
- The functions that return a HTTN on the controller thread (e.g. GetTargetNode and GetTargetAPZC) would set the flag before returning the HTTN
- The caller would clear the flag when it's done with the HTTN

As a mechanism to make sure the flag is set/cleared correctly we could introduce a HitTestingTreeNodeRef structure that stores a RefPtr to the HitTestingTreeNode and manages the flag in a RAII fashion. This type would be the thing returned from GetTargetNode/GetTargetAPZC instead of the RefPtr<HitTestingTreeNode> that is currently used. Furthermore we can make things like HTTN::GetScrollbarData private and only accessible via the HTTNRef, to ensure that we only access it while the flag is set and the node is not in danger of getting recycled.

Comment 12

[Intermittent Failures Robot]

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

Repository breakdown:
* mozilla-central: 1

Platform breakdown:
* linux64-ccov: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1457590&startday=2018-05-21&endday=2018-05-27&tree=trunk

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=317dfb9c05c2d01337b738bd747d19488782a71d

Comment 15

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

Attachment: Bug 1457590 - Strengthen the contract around recycling HitTestingTreeNodes.

This patch makes three related changes:
- A non-functional change that factors a IsRecyclable function on
  HitTestingTreeNode.
- A non-functional change that sprinkles proof-of-tree-lock arguments to
  a few HitTestingTreeNode functions, to ensure at compile-time that they
  can only be called while holding the tree lock.
- A functional change that stops clearing mLayersId in
  HitTestingTreeNode::Destroy, so that if a node is non-recyclable, and
  it gets Destroy()'d while other code still has a RefPtr to it, that
  other code can still read the layers id off in a safe manner.

These changes provide a stronger set of checks around node recycling,
and allows for a safe mechanism to use a HitTestingTreeNode on the
controller thread without having to lock the entire APZ tree. The
mechanism is effectively a per-node lock, which will be added in the
next patch.

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

Comment 16

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

Attachment: Bug 1457590 - Add the HitTestingTreeNodeAutoLock class.

This adds the HitTestingTreeNodeAutoLock RAII class that allows using
a HitTestingTreeNode safely outside a tree lock, and ensures that the
node won't get destroyed or recycled concurrently.

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

Comment 17

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

Attachment: Bug 1457590 - Use the HitTestingTreeNodeAutoLock.

This updates the GetTargetAPZC function to produce a
HitTestingTreeNodeAutoLock out-param instead of a
RefPtr<HitTestingTreeNode>, to ensure that the node can be used safely
in calling functions. It then propagates that change outward as needed,
which covers all the scrollbar dragging code.

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

Comment 18

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

I couldn't find a way to reliably reproduce this, but we can monitor the effectiveness of the patches via crash-stats, since we are seeing these crashes in the wild.

Also, sorry if I went a little overboard with the aProofOfTreeLock business, but I figured it was better than documenting it in comments since this way it's compiler-enforceable.

Comment 19

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

Update to fix the static analysis compile error: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c74c0bddbc5ab3a94472d4dab7b571e6da40837

Comment 20

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

Comment on attachment 8981996 [details]
Bug 1457590 - Strengthen the contract around recycling HitTestingTreeNodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248048/diff/1-2/

Comment 21

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

Comment on attachment 8981997 [details]
Bug 1457590 - Add the HitTestingTreeNodeAutoLock class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248050/diff/1-2/

Comment 22

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

Comment on attachment 8981998 [details]
Bug 1457590 - Use the HitTestingTreeNodeAutoLock.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248052/diff/1-2/

Comment 23

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

... and with the silly-null-deref fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aa96a507724617e6531b72ba7ab0c7fbc858e46

Comment 24

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

Comment on attachment 8981996 [details]
Bug 1457590 - Strengthen the contract around recycling HitTestingTreeNodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248048/diff/2-3/

Comment 25

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

Comment on attachment 8981997 [details]
Bug 1457590 - Add the HitTestingTreeNodeAutoLock class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248050/diff/2-3/

Comment 26

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

Comment on attachment 8981998 [details]
Bug 1457590 - Use the HitTestingTreeNodeAutoLock.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248052/diff/2-3/

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 8981996 [details]
Bug 1457590 - Strengthen the contract around recycling HitTestingTreeNodes.

https://reviewboard.mozilla.org/r/248048/#review254198

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8981997 [details]
Bug 1457590 - Add the HitTestingTreeNodeAutoLock class.

https://reviewboard.mozilla.org/r/248050/#review254204

::: gfx/layers/apz/src/HitTestingTreeNode.cpp:456
(Diff revision 3)
> +    return;
> +  }
> +  MOZ_ASSERT(mTreeMutex);
> +
> +  { // scope lock
> +    RecursiveMutexAutoLock lock(*mTreeMutex);

Are we sure a node will never outlive the APZCTM (and thus its mutex)?

Comment 29

[Botond Ballo [:botond]]

Comment on attachment 8981998 [details]
Bug 1457590 - Use the HitTestingTreeNodeAutoLock.

https://reviewboard.mozilla.org/r/248052/#review254222

Nice and clean, I like it!

Comment 30

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

(In reply to Botond Ballo [:botond] from comment #28)
> Are we sure a node will never outlive the APZCTM (and thus its mutex)?

Fairly sure, yeah. Conceptually the APZCTM owns the nodes, and we don't have any code outside APZ that touches the HitTestingTreeNode. Any operations that hold refptrs to nodes should be going through the APZCTreeManager and have that on the stack, so the mutex should still be valid at that point. If we run into situations where that's not the case I think we'd have bigger problems.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a1bdb8d8bf5
Strengthen the contract around recycling HitTestingTreeNodes. r=botond
https://hg.mozilla.org/integration/autoland/rev/437b901404db
Add the HitTestingTreeNodeAutoLock class. r=botond
https://hg.mozilla.org/integration/autoland/rev/85d13ab240fa
Use the HitTestingTreeNodeAutoLock. r=botond

Comment 32

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8a1bdb8d8bf5
https://hg.mozilla.org/mozilla-central/rev/437b901404db
https://hg.mozilla.org/mozilla-central/rev/85d13ab240fa

Comment 33

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

I discussed with with RyanVM on IRC - there is a real correctness issue affecting older versions, which then got exposed as an assertion failure due to recent changes. And it would be nice to fix that correctness issue, but the patch isn't totally trivial either. I'd prefer to let it bake for a bit, and considering we're close to the end of the current cycle we might as well let it ride the trains.

Comment 34

[Intermittent Failures Robot]

5 failures in 655 pushes (0.008 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 2
* autoland: 2
* mozilla-beta: 1

Platform breakdown:
* linux64-qr: 4
* linux64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1457590&startday=2018-05-28&endday=2018-06-03&tree=trunk