Closed
Bug 1389335
Opened 7 years ago
Closed 7 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)
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)
Filed by: wkocher [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=122221944&repo=autoland https://queue.taskcluster.net/v1/task/Rx1LnamITU-Zti2j5TOA4Q/runs/0/artifacts/public/logs/live_backing.log
Updated•7 years ago
|
Flags: needinfo?(rhunt)
OS: Unspecified → Linux
Hardware: Unspecified → x86
Whiteboard: [gfx-noted]
Version: unspecified → 57 Branch
Assignee | ||
Comment 1•7 years 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)
Comment 2•7 years 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. 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 hidden (Intermittent Failures Robot) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
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•7 years 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•7 years 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•7 years ago
|
Assignee: nobody → rhunt
Assignee | ||
Comment 7•7 years 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•7 years 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!
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/847e6f19eac2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Blocks: 1377707
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Version: 57 Branch → 56 Branch
Comment 11•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0537e2f37ee1
You need to log in
before you can comment on or make changes to this bug.
Description
•