Closed Bug 1419917 Opened 2 years ago Closed 2 years ago

Assertion failure: old == oldItem, at /builds/worker/workspace/build/src/layout/painting/RetainedDisplayListBuilder.cpp:450

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: jkratzer, Assigned: mtseng)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file trigger.html
Testcase found while fuzzing mozilla-central rev 5378dcb45044.

OS|Linux|0.0.0 Linux 4.4.0-98-generic #121-Ubuntu SMP Tue Oct 10 14:24:03 UTC 2017 x86_64
CPU|amd64|family 6 model 78 stepping 3|1
GPU|||
Crash|SIGSEGV|0x0|0
0|0|libxul.so|RetainedDisplayListBuilder::MergeDisplayLists|hg:hg.mozilla.org/mozilla-central:layout/painting/RetainedDisplayListBuilder.cpp:5378dcb45044|450|0x0
0|1|libxul.so|RetainedDisplayListBuilder::AttemptPartialUpdate|hg:hg.mozilla.org/mozilla-central:layout/painting/RetainedDisplayListBuilder.cpp:5378dcb45044|880|0x12
0|2|libxul.so|nsLayoutUtils::PaintFrame|hg:hg.mozilla.org/mozilla-central:layout/base/nsLayoutUtils.cpp:5378dcb45044|3860|0xe
0|3|libxul.so|mozilla::PresShell::Paint|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:5378dcb45044|6513|0x17
0|4|libxul.so|nsViewManager::ProcessPendingUpdatesPaint|hg:hg.mozilla.org/mozilla-central:view/nsViewManager.cpp:5378dcb45044|480|0x12
0|5|libxul.so|nsViewManager::ProcessPendingUpdatesForView|hg:hg.mozilla.org/mozilla-central:view/nsViewManager.cpp:5378dcb45044|412|0xd
0|6|libxul.so|nsViewManager::ProcessPendingUpdates|hg:hg.mozilla.org/mozilla-central:view/nsViewManager.cpp:5378dcb45044|1102|0x11
0|7|libxul.so|nsRefreshDriver::Tick|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5378dcb45044|2027|0x8
0|8|libxul.so|mozilla::RefreshDriverTimer::TickRefreshDrivers|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5378dcb45044|306|0xf
0|9|libxul.so|mozilla::RefreshDriverTimer::Tick|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5378dcb45044|328|0x12
0|10|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5378dcb45044|769|0x5
0|11|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:5378dcb45044|583|0xc
0|12|libxul.so|mozilla::layout::VsyncChild::RecvNotify|hg:hg.mozilla.org/mozilla-central:layout/ipc/VsyncChild.cpp:5378dcb45044|68|0x9
0|13|libxul.so|mozilla::layout::PVsyncChild::OnMessageReceived|s3:gecko-generated-sources:375d0a11c0f73d4058318d5ce2ca6f95774c2ad12c187859ba0bfc07a6e9f17429c4d3628eb03e74766cf642501f10b1ed7f58baddeb4fb20e1277fe9bfed7bd/ipc/ipdl/PVsyncChild.cpp:|155|0xf
0|14|libxul.so|mozilla::ipc::MessageChannel::DispatchAsyncMessage|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:5378dcb45044|2114|0x6
0|15|libxul.so|mozilla::ipc::MessageChannel::DispatchMessage|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:5378dcb45044|2044|0xb
0|16|libxul.so|mozilla::ipc::MessageChannel::RunMessage|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:5378dcb45044|1890|0xb
0|17|libxul.so|mozilla::ipc::MessageChannel::MessageTask::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:5378dcb45044|1923|0xc
0|18|libxul.so|nsThread::ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:5378dcb45044|1037|0x15
0|19|libxul.so|NS_ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:5378dcb45044|513|0x11
0|20|libxul.so|mozilla::ipc::MessagePump::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:5378dcb45044|97|0xa
0|21|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:5378dcb45044|326|0x17
0|22|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:5378dcb45044|319|0x8
0|23|libxul.so|nsBaseAppShell::Run|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:5378dcb45044|159|0xd
0|24|libxul.so|XRE_RunAppShell|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:5378dcb45044|877|0x11
0|25|libxul.so|mozilla::ipc::MessagePumpForChildProcess::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:5378dcb45044|269|0x5
0|26|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:5378dcb45044|326|0x17
0|27|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:5378dcb45044|319|0x8
0|28|libxul.so|XRE_InitChildProcess|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:5378dcb45044|703|0x8
0|29|firefox|content_process_main|hg:hg.mozilla.org/mozilla-central:ipc/contentproc/plugin-container.cpp:5378dcb45044|63|0x14
0|30|firefox|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:5378dcb45044|280|0x11
0|31|libc-2.23.so||||0x20830
0|32|firefox|MOZ_ReportAssertionFailure|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.h:5378dcb45044|165|0x5
Flags: in-testsuite?
Attached file prefs.js
This happens because nsDisplayThemedBackground doesn't have a table specific version to handle us creating multiple for the same frame. We need nsDisplayTableThemedbackground.

Morris, can you fix this?
Flags: needinfo?(mtseng)
Sure. I'll handle this.
Assignee: nobody → mtseng
Flags: needinfo?(mtseng)
Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

https://reviewboard.mozilla.org/r/202750/#review208072

::: layout/painting/nsDisplayList.h:3943
(Diff revision 1)
> +  virtual uint32_t GetPerFrameKey() const override {
> +    return (static_cast<uint8_t>(mTableType) << TYPE_BITS) |
> +           nsDisplayItem::GetPerFrameKey();
> +  }
> +
> +  virtual nsIFrame* FrameForInvalidation() const override { return mAncestorFrame; }

Don't we also need to override StyleFrame() and make sure the base class uses that whenever it needs style data?
Attachment #8931603 - Flags: review?(matt.woodrow)
Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

https://reviewboard.mozilla.org/r/202750/#review208072

> Don't we also need to override StyleFrame() and make sure the base class uses that whenever it needs style data?

Good catch. Thanks. I've fixed this.
Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

https://reviewboard.mozilla.org/r/202750/#review208358


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: layout/painting/nsDisplayList.cpp:4251
(Diff revision 2)
>    , mBackgroundRect(aBackgroundRect)
>  {
>    MOZ_COUNT_CTOR(nsDisplayThemedBackground);
> +}
> +
> +nsDisplayThemedBackground::~nsDisplayThemedBackground()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

https://reviewboard.mozilla.org/r/202750/#review208392
Attachment #8931603 - Flags: review?(matt.woodrow) → review+
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ea21d0761dd
Add nsDisplayTableThemedBackground. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/2ea21d0761dd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Can we land this testcase as a crashtest please? Also, do we want to uplift this to Beta in support of the retained DL SHIELD study being planned?
Flags: needinfo?(mtseng)
Version: 52 Branch → unspecified
Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: The tab will crash.
[Is this code covered by automated tests?]: No, but I'll add crashtest later.
[Has the fix been verified in Nightly?]: Yap, I verified it in my local.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just add a new displayitem for table's themed background. No other code change. Therefore, the code change is minimum. Should be low risk.
[String changes made/needed]: No
Flags: needinfo?(mtseng)
Attachment #8931603 - Flags: approval-mozilla-beta?
I'll add the crashtest.
Duplicate of this bug: 1419869
Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

Fix an assertion failure related to retain display list. Beta58+.
Attachment #8931603 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Backed out for bustage: https://hg.mozilla.org/releases/mozilla-beta/rev/f402c67ee9b03e2057afee139eea99456749b65e

There was a conflict applying the patch, maybe more code needs to be updated.
Flags: needinfo?(mtseng)
MozReview-Commit-ID: 9IS2ehWvPHg
Attachment #8934454 - Flags: review+
Comment on attachment 8934454 [details] [diff] [review]
Add nsDisplayTableThemedBackground. (patch for beta)

This is patch for beta. :aryx, could you land it again? Thanks.
Attachment #8934454 - Attachment description: Add nsDisplayTableThemedBackground. → Add nsDisplayTableThemedBackground. (patch for beta)
Flags: needinfo?(mtseng) → needinfo?(aryx.bugmail)
Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.