Closed Bug 1447131 Opened 6 years ago Closed 6 years ago

Crash in mozilla::layers::InputBlockState::SetConfirmedTargetApzc

Categories

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

Product:
Core
Component:
Panning and Zooming
Version:
60 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + fixed
firefox61 + fixed

People

(Reporter: philipp, Assigned: botond)

References

Details

(Keywords: crash, regression, Whiteboard: [gfx-noted])

Crash Data

Attachments

(5 files, 1 obsolete file)

Reduced testcase
675 bytes, text/html
Details
Display list dump
18.63 KB, text/plain
Details
Bug 1447131 - Handle backface-visibility:hidden in compositor hit testing.
59 bytes, text/x-review-board-request
kats
: review+
RyanVM
: approval-mozilla-beta+
Details
Bug 1447131 - Move centerOf() into apz_test_utils.js.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1447131 - Mochitest for hit-testing over backface-visibility:hidden element.
59 bytes, text/x-review-board-request
kats
: review+
Details 
This bug was filed from the Socorro interface and is
report bp-54681e6b-2355-44eb-b1fd-cb69b0180319.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::layers::InputBlockState::SetConfirmedTargetApzc gfx/layers/apz/src/InputBlockState.cpp:72
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:742
3 xul.dll mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::layers::IAPZCTreeManager>, void  xpcom/threads/nsThreadUtils.h:1200
4 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1040
5 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:517
6 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:125
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:319
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:299
9 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:157

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

crashes with this signature started regressing cross-platform in firefox 60. half of them are recorded in the main browser process, the other half in the gpu process. these crashes happen with a MOZ_DIAGNOSTIC_ASSERT(false, "APZ and main thread confirmed scrollbar drag block with different targets") that was added in bug 1437694.

the patch from bug 1443518 has already decreased the volume of this crash, but it's still around for nightly and devedition users in sizable numbers...
Assignee: nobody → botond
Following up from bug 1443518 comment 27:

> Additional URLs that triggered this crash have been provided 
> to me privately, and I plan to test them and reduce them as 
> appropriate.

I went through all of the URLs that have been provided to me, and I can't reproduce this crash on any of them with the fix for bug 1443518 in place.

If you're still experiencing this crash with the fix in bug 1443518 in place (that would be on a Nightly build from March 14 onwards, or a Beta build from March 16 onwards), please provide STR or a URL (either here, or email me). Thanks!
(In reply to Botond Ballo [:botond] from comment #1)
> Following up from bug 1443518 comment 27:
> 
> > Additional URLs that triggered this crash have been provided 
> > to me privately, and I plan to test them and reduce them as 
> > appropriate.
> 
> I went through all of the URLs that have been provided to me, and I can't
> reproduce this crash on any of them with the fix for bug 1443518 in place.
> 
> If you're still experiencing this crash with the fix in bug 1443518 in place
> (that would be on a Nightly build from March 14 onwards, or a Beta build
> from March 16 onwards), please provide STR or a URL (either here, or email
> me). Thanks!

I can easily reproduce this crash on both Windows and Linux with a Nightly from after March 14.
For me it happens, when I scroll a mail in SoGO (https://sogo.nu/), as it is my private mailbox I can't provide access to it.
(In reply to Kristian Klausen from comment #2)
> I can easily reproduce this crash on both Windows and Linux with a Nightly
> from after March 14.
> For me it happens, when I scroll a mail in SoGO (https://sogo.nu/), as it is
> my private mailbox I can't provide access to it.

I tried one of the test accounts mentioned at the bottom of https://sogo.nu/. Sent myself an email that's long enough to scroll and tried to scroll it, both when composing it and when reading it, but couldn't trigger the crash. I tried both v2 and v3 of the app.

I also tried several dozen links that Calixte sent me privately, and wasn't able to trigger the crash with them, either. (Granted, those are just URLs, not STR, so I had to guess what STR to try (given where the crash is, it ought to be "try dragging the scrollbar of a subframe on the page". Also some of the pages require having login credentials to a site.)

I tried all of the above on both Linux and Windows.

If anyone else who is experiencing this issue could provide steps to reproduce the crash, that would be very helpful!
(In reply to Botond Ballo [:botond] from comment #3)
> (In reply to Kristian Klausen from comment #2)
> > I can easily reproduce this crash on both Windows and Linux with a Nightly
> > from after March 14.
> > For me it happens, when I scroll a mail in SoGO (https://sogo.nu/), as it is
> > my private mailbox I can't provide access to it.
> 
> I tried one of the test accounts mentioned at the bottom of
> https://sogo.nu/. Sent myself an email that's long enough to scroll and
> tried to scroll it, both when composing it and when reading it, but couldn't
> trigger the crash. I tried both v2 and v3 of the app.

I couldn't reproduce the crash on SoGO demo instance either, but I can easily reproduce it on my own setup. I did send you a mail yesterday with account credentials to a dummy mail acc on my SoGO setup.
(In reply to Kristian Klausen from comment #4)
> I did send you a mail yesterday with
> account credentials to a dummy mail acc on my SoGO setup.

Ah, that email went into my spam folder and I didn't notice it :)

I am able to reproduce the crash with your SoGO instance. Much thanks!
Priority: -- → P2
Whiteboard: [gfx-noted]
Attached file Reduced testcase — 
Here is a reduced testcase that demonstrates the problem. The issue seems to be related to the use of |backface-visibility:hidden|.
Attachment #8961950 - Attachment mime type: text/plain → text/html
Unlike bug 1443518, this testcase is broken with both CompositorHitTestInfo and LayerEventRegions. Indeed, the scrollbar in this testcase can't be dragged in Firefox 59 (or any version since Firefox 55 with e10s enabled). (Which means that, regardless of whether the underlying hit testing bug gets fixed for Firefox 60, bug 1437694 will have fixed a bad scrollbar dragging issue in Firefox 60, which was exactly my intention.)
Attached file Display list dump — 
Here's a display list dump (with LayerEventRegions, but it doesn't really matter). Notice the layer 0x7f767c4c8800 is marked "[not visible]", but nonetheless has a hit region that's eating the events.
It's worth noting that the underlying hit testing bug affects not only scrollbar dragging in this testcase, but also wheel scrolling (and presumably touch scrolling as well).
The attached patch fixes the issue for me.

WebRender hit testing will need a separate fix, as the fix is on the compositor side, not the display list building side.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=187942611fa51d572f18b68231949b2f36cb739c
Comment on attachment 8961984 [details]
Bug 1447131 - Handle backface-visibility:hidden in compositor hit testing.

https://reviewboard.mozilla.org/r/230828/#review236824
Attachment #8961984 - Flags: review?(bugmail) → review+
Comment on attachment 8962536 [details]
Bug 1447131 - Move centerOf() into apz_test_utils.js.

https://reviewboard.mozilla.org/r/231338/#review236826
Attachment #8962536 - Flags: review?(bugmail) → review+
Comment on attachment 8962537 [details]
Bug 1447131 - Mochitest for hit-testing over backface-visibility:hidden element.

https://reviewboard.mozilla.org/r/231340/#review236828

Thanks for the test!
Attachment #8962537 - Flags: review?(bugmail) → review+
Now with WebRender hit testing fixed as well. (Thanks for the tip, Kats - doing what you suggested basically worked right off the bat!)
I guess one potential concern is whether it's ok to increase the size of every HitTestItem like this, or if we should try to pack that bit of information in some existing place.
Comment on attachment 8962545 [details]
Bug 1447131 - Handle backface-visibility:hidden in WebRender hit testing.

https://reviewboard.mozilla.org/r/231346/#review237046

This will need to land as a PR into webrender. mrobinson should probably review, and he can advise you if there's a better/more efficient way to do it - maybe there's a way to skip building the HitTestItem for these items entirely?
Attachment #8962545 - Flags: review?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> Comment on attachment 8962545 [details]
> Bug 1447131 - Handle backface-visibility:hidden in WebRender hit testing.
> 
> https://reviewboard.mozilla.org/r/231346/#review237046
> 
> This will need to land as a PR into webrender. mrobinson should probably
> review, and he can advise you if there's a better/more efficient way to do
> it - maybe there's a way to skip building the HitTestItem for these items
> entirely?

Moved this patch over to https://github.com/servo/webrender/pull/2585.
Attachment #8962545 - Attachment is obsolete: true
Reverted to the original patch series which doesn't include the WR fix, and disables the test on WR. I'm going to land that now, to stop crashing people using non-WR, and then enable the test for WR in a follow-up bug once the WR PR merges to Gecko.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4b8a1682e6c
Handle backface-visibility:hidden in compositor hit testing. r=kats
https://hg.mozilla.org/integration/autoland/rev/baa7a07aaf64
Move centerOf() into apz_test_utils.js. r=kats
https://hg.mozilla.org/integration/autoland/rev/cb85ee2a04fa
Mochitest for hit-testing over backface-visibility:hidden element. r=kats
Blocks: 1449738
(In reply to Botond Ballo [:botond] from comment #29)
> Reverted to the original patch series which doesn't include the WR fix, and
> disables the test on WR. I'm going to land that now, to stop crashing people
> using non-WR, and then enable the test for WR in a follow-up bug once the WR
> PR merges to Gecko.

That follow-up bug is bug 1449738.
Blocks: 1449746
Looks like there's still crash reports coming in from builds which include this fix. Are they the same issue or something else which should be tracked in a new bug?

https://crash-stats.mozilla.com/report/index/3892a23e-b3af-4224-8975-7c4670180330
https://crash-stats.mozilla.com/report/index/b8b04f29-d278-48ae-8510-5c3760180330
tracking-firefox60: --- → +
tracking-firefox61: --- → +
Flags: needinfo?(botond)
Flags: in-testsuite+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> Looks like there's still crash reports coming in from builds which include
> this fix. Are they the same issue or something else which should be tracked
> in a new bug?
> 
> https://crash-stats.mozilla.com/report/index/3892a23e-b3af-4224-8975-
> 7c4670180330
> https://crash-stats.mozilla.com/report/index/b8b04f29-d278-48ae-8510-
> 5c3760180330

The diagnostic assert with this signature that was added in bug 1437694 can have multiple underlying causes. We fixed one in bug 1443518, and another in this bug, but there could be others.

Looking at the linked crash reports, one of them has WebRender enabled, so it could be this bug (whose WebRender part is yet to land in the next WebRender update (bug 1449562)). The other doesn't have WebRender enabled, so it could be a different underlying cause. I would suggest filing a new bug to track that.

Note also that the "crash" is MOZ_DIAGNOSTIC_ASSERT, which only fires in nightly and early beta builds. As the crash will never make it to release builds (nor beta builds which are not "early beta"), I'm not sure whether we need to track this.
Flags: needinfo?(botond)
On a different note, we should probably uplift this bug to Firefox 60, because even though the crash won't make it to release, the underlying hit-testing bug affects not only scrollbar dragging (for which the the recovery codepath added in bug 1437694 will kick in), but also wheel scrolling (which has no recovery codepath).
Comment on attachment 8961984 [details]
Bug 1447131 - Handle backface-visibility:hidden in compositor hit testing.

Approval Request Comment
[Feature/Bug causing the regression]:
  APZ. (The diagnostic assert that made us discover the
  issue was added in bug 1437694, but the underlying
  hit testing bug has been there since APZ was enabled.) 

[User impact if declined]:
  User cannot scroll a scrollframe with the mousewheel
  (and likely touchpad / touchscreen as well)

[Is this code covered by automated tests?]:
  Yes, added in this bug.

[Has the fix been verified in Nightly?]:
  Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
  No.

[List of other uplifts needed for the feature/fix]:
  None.

[Is the change risky?]:
  Low risk.

[Why is the change risky/not risky?]:
  The change is straightforward, and only affects elements
  that are backface-visibility: hidden.

[String changes made/needed]:
  None.
Attachment #8961984 - Flags: approval-mozilla-beta?
(In reply to Botond Ballo [:botond] from comment #36)
> [User impact if declined]:
>   User cannot scroll a scrollframe with the mousewheel
>   (and likely touchpad / touchscreen as well)

Whoops, I didn't finish writing that. That was meant to be:

   User cannot scroll a scrollframe with the mousewheel
   (and likely touchpad / touchscreen as well) if it has
   a backface-visibility:hidden element in front of it.
No longer blocks: 1449746
Depends on: 1449746
Comment on attachment 8961984 [details]
Bug 1447131 - Handle backface-visibility:hidden in compositor hit testing.

Fix some APZ scrolling issues. Approved for 60.0b9.
Attachment #8961984 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I crashed 61.0a1 with a build 20180410100115 after these patches landed bp-e4678157-12f0-4986-b4fc-fc8f70180427
(In reply to Wayne Mery (:wsmwk) from comment #40)
> I crashed 61.0a1 with a build 20180410100115 after these patches landed
> bp-e4678157-12f0-4986-b4fc-fc8f70180427

That's a fairly old build, it might have been fixed by bug 1443424. However even with that bug fixed we know there are additional root causes that can trigger this crash. bug 1455860 restricts the crash to Nightly, but that won't help you since you're already on Nightly. If you have a specific page on which you can reproduce this, please file a new bug with steps to reproduce as that would be very helpful. Thanks!
See Also: → 1443424, 1455860
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: