Intermittent gfx/layers/apz/test/mochitest/test_key_scroll.html,test_bug1151667.html | application crashed [@ mozilla::layers::APZTestData::LogTestDataImpl] after Assertion failure: gfxPrefs::APZTestLoggingEnabled() (don't call me), at APZTestData.h:89

RESOLVED FIXED in Firefox 56

Status

()

Core
Panning and Zooming
P5
critical
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Treeherder Bug Filer, Assigned: rhunt)

Tracking

({crash, intermittent-failure})

56 Branch
mozilla57
x86
Linux
crash, intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [gfx-noted], crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Flags: needinfo?(rhunt)
OS: Unspecified → Linux
Hardware: Unspecified → x86
Whiteboard: [gfx-noted]
Version: unspecified → 57 Branch
(Assignee)

Comment 1

11 months ago
Huh, I'm not sure entirely how this is possible. The assertion is that APZTestLogging is enabled when logging happens inside APZTestData.

We construct a APZPaintLogHelper backed with APZTestData if APZTestLogging is enabled or nullptr if not [1] and then use that for logging [2].

My best guess is that the pref changes asynchronously between the creation of APZPaintLogHelper and logging. If that's the case we should only assert for APZTestLogging in the constructor of APZPaintLogHelper or APZTestData.

I also noticed this before the test:

"[Parent 1092] WARNING: [nsFrameLoader] ReallyStartLoadingInternal tried but couldn't show remote browser."

Not sure how that affects this, maybe it changed timing and the pref change propagates faster.

[1] http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/gfx/layers/apz/src/APZCTreeManager.cpp#285
[2] http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/gfx/layers/apz/src/APZCTreeManager.cpp#859
Flags: needinfo?(rhunt)
(In reply to Ryan Hunt [:rhunt] from comment #1)
> My best guess is that the pref changes asynchronously between the creation
> of APZPaintLogHelper and logging. If that's the case we should only assert
> for APZTestLogging in the constructor of APZPaintLogHelper or APZTestData.

That sounds right to me. Botond, any objections?
Flags: needinfo?(botond)

Comment 3

11 months ago
2 failures in 901 pushes (0.002 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 2

Platform breakdown:
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1389335&startday=2017-08-07&endday=2017-08-13&tree=all
Comment hidden (mozreview-request)
Summary: Intermittent gfx/layers/apz/test/mochitest/test_key_scroll.html | application crashed [@ mozilla::layers::APZTestData::LogTestDataImpl] after Assertion failure: gfxPrefs::APZTestLoggingEnabled() (don't call me), at APZTestData.h:89 → Intermittent gfx/layers/apz/test/mochitest/test_key_scroll.html,test_bug1151667.html | application crashed [@ mozilla::layers::APZTestData::LogTestDataImpl] after Assertion failure: gfxPrefs::APZTestLoggingEnabled() (don't call me), at APZTestData.h:89

Comment 5

11 months ago
(In reply to Ryan Hunt [:rhunt] from comment #1)
> My best guess is that the pref changes asynchronously between the creation
> of APZPaintLogHelper and logging.

Yeah, I guess that could happen! Mochitests enable the pref via pushPrefs() (e.g. [1]), which will restore the previous pref state (disabling this one) at the end of the state. That message could arrive to the compositor while an UpdateHitTestingTree operation is in progress.

[1] http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/gfx/layers/apz/test/mochitest/test_bug1151663.html#22
Flags: needinfo?(botond)

Comment 6

11 months ago
mozreview-review
Comment on attachment 8896837 [details]
Bug 1389335 - Only check APZTestDataEnabled when APZPaintLogHelper is created.

https://reviewboard.mozilla.org/r/168136/#review173614

It's a bit unfortunate to have to remove the assert from LogTestDataImpl() as that function can have other callers than APZPaintLogHelper, but we also have asserts in both of the other callers than currently exist (ClientLayerManager::LogTestDataForCurrentPaint() and WebRenderLayerManager::LogTestDataForCurrentPaint()), so I guess this is fine.
Attachment #8896837 - Flags: review?(botond) → review+
(Assignee)

Updated

11 months ago
Assignee: nobody → rhunt
(Assignee)

Comment 7

11 months ago
mozreview-review
Comment on attachment 8896837 [details]
Bug 1389335 - Only check APZTestDataEnabled when APZPaintLogHelper is created.

https://reviewboard.mozilla.org/r/168136/#review173630

::: gfx/layers/apz/testutil/APZTestData.h:108
(Diff revision 1)
>  class APZPaintLogHelper {
>  public:
>    APZPaintLogHelper(APZTestData* aTestData, SequenceNumber aPaintSequenceNumber)
>      : mTestData(aTestData),
> -      mPaintSequenceNumber(aPaintSequenceNumber)
> -  {}
> +      mPaintSequenceNumber(aPaintSequenceNumber) {
> +    MOZ_ASSERT(gfxPrefs::APZTestLoggingEnabled(), "don't call me");

I don't think this will work...

APZPaintLogHelper will be created regardless of APZTestLoggingEnabled().

The correct condition is:

MOZ_ASSERT(!aTestData || gfxPrefs::APZTestLoggingEnabled())

Comment 8

11 months ago
(In reply to Ryan Hunt [:rhunt] from comment #7)
> ::: gfx/layers/apz/testutil/APZTestData.h:108
> (Diff revision 1)
> >  class APZPaintLogHelper {
> >  public:
> >    APZPaintLogHelper(APZTestData* aTestData, SequenceNumber aPaintSequenceNumber)
> >      : mTestData(aTestData),
> > -      mPaintSequenceNumber(aPaintSequenceNumber)
> > -  {}
> > +      mPaintSequenceNumber(aPaintSequenceNumber) {
> > +    MOZ_ASSERT(gfxPrefs::APZTestLoggingEnabled(), "don't call me");
> 
> I don't think this will work...
> 
> APZPaintLogHelper will be created regardless of APZTestLoggingEnabled().
> 
> The correct condition is:
> 
> MOZ_ASSERT(!aTestData || gfxPrefs::APZTestLoggingEnabled())

Indeed. My bad, I should have looked at this part of the patch more carefully!

Comment 9

11 months ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/847e6f19eac2
Only check APZTestDataEnabled when APZPaintLogHelper is created. r=botond

Comment 10

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/847e6f19eac2
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1377707
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
Version: 57 Branch → 56 Branch

Comment 11

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/0537e2f37ee1
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.