Closed Bug 1389335 Opened 2 years ago Closed 2 years ago

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

Categories

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

56 Branch
x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: rhunt)

References

Details

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

Crash Data

Attachments

(1 file)

Flags: needinfo?(rhunt)
OS: Unspecified → Linux
Hardware: Unspecified → x86
Whiteboard: [gfx-noted]
Version: unspecified → 57 Branch
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)
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
(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 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: nobody → rhunt
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())
(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!
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/847e6f19eac2
Only check APZTestDataEnabled when APZPaintLogHelper is created. r=botond
https://hg.mozilla.org/mozilla-central/rev/847e6f19eac2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1377707
Version: 57 Branch → 56 Branch
You need to log in before you can comment on or make changes to this bug.