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

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: 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

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: prefs.js

Comment 2

[Matt Woodrow (:mattwoodrow)]

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?

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Sure. I'll handle this.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Bug 1419917 - Add nsDisplayTableThemedBackground.

Review commit: https://reviewboard.mozilla.org/r/202750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202750/

Comment 5

[Matt Woodrow (:mattwoodrow)]

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?

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202750/diff/1-2/

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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 8

[Code Review Bot [:reviewbot]]

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 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ea21d0761dd
Add nsDisplayTableThemedBackground. r=mattwoodrow

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2ea21d0761dd

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I'll add the crashtest.

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8931603 [details]
Bug 1419917 - Add nsDisplayTableThemedBackground.

Fix an assertion failure related to retain display list. Beta58+.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3da0dff84972

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 19

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Add nsDisplayTableThemedBackground. (patch for beta)

MozReview-Commit-ID: 9IS2ehWvPHg

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8934454 [details] [diff] [review]
Add nsDisplayTableThemedBackground. (patch for beta)

This is patch for beta. :aryx, could you land it again? Thanks.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/91082bb1dc37