1464641 - crash at null in [@ MergeState::HasMatchingItemInOldList]

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

==6169==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x00000055fed8 bp 0x7ffe680fe5b0 sp 0x7ffe680fe440 T0)
==6169==The signal is caused by a WRITE memory access.
==6169==Hint: address points to the zero page.
    #0 0x55fed7 in MOZ_CrashPrintf src/mfbt/Assertions.cpp:63:3
    #1 0x7f713dbca26d in GetOldListIndex src/layout/painting/nsDisplayList.h:2871:7
    #2 0x7f713dbca26d in MergeState::HasMatchingItemInOldList(nsDisplayItem*, Index<OldListUnits>*) src/layout/painting/RetainedDisplayListBuilder.cpp:338
    #3 0x7f713dac289e in MergeState::ProcessItemFromNewList(nsDisplayItem*, mozilla::Maybe<Index<MergedListUnits> > const&) src/layout/painting/RetainedDisplayListBuilder.cpp:284:9
    #4 0x7f713dac1d98 in RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList*, RetainedDisplayList*, RetainedDisplayList*, mozilla::Maybe<mozilla::ActiveScrolledRoot const*>&, unsigned int) src/layout/painting/RetainedDisplayListBuilder.cpp:513:36
    #5 0x7f713dac2cea in MergeState::ProcessItemFromNewList(nsDisplayItem*, mozilla::Maybe<Index<MergedListUnits> > const&) src/layout/painting/RetainedDisplayListBuilder.cpp:292:25
    #6 0x7f713dac1d98 in RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList*, RetainedDisplayList*, RetainedDisplayList*, mozilla::Maybe<mozilla::ActiveScrolledRoot const*>&, unsigned int) src/layout/painting/RetainedDisplayListBuilder.cpp:513:36
    #7 0x7f713dac2cea in MergeState::ProcessItemFromNewList(nsDisplayItem*, mozilla::Maybe<Index<MergedListUnits> > const&) src/layout/painting/RetainedDisplayListBuilder.cpp:292:25
    #8 0x7f713dac1d98 in RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList*, RetainedDisplayList*, RetainedDisplayList*, mozilla::Maybe<mozilla::ActiveScrolledRoot const*>&, unsigned int) src/layout/painting/RetainedDisplayListBuilder.cpp:513:36
    #9 0x7f713dacaec1 in RetainedDisplayListBuilder::AttemptPartialUpdate(unsigned int, mozilla::DisplayListChecker*) src/layout/painting/RetainedDisplayListBuilder.cpp:1205:7
    #10 0x7f713d28238b in nsLayoutUtils::PaintFrame(gfxContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) src/layout/base/nsLayoutUtils.cpp:3696:40
    #11 0x7f713d1774d7 in mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) src/layout/base/PresShell.cpp:6316:5
    #12 0x7f713cb0cb1a in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) src/view/nsViewManager.cpp:480:19
    #13 0x7f713cb0b91c in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) src/view/nsViewManager.cpp:412:33
    #14 0x7f713cb10f76 in nsViewManager::ProcessPendingUpdates() src/view/nsViewManager.cpp:1102:5
    #15 0x7f713d0f0dc5 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:2039:11
    #16 0x7f713d0fdb9b in TickDriver src/layout/base/nsRefreshDriver.cpp:328:13
    #17 0x7f713d0fdb9b in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:301
    #18 0x7f713d0fd779 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:320:5
    #19 0x7f713d1002be in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:760:5
    #20 0x7f713d1002be in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:673
    #21 0x7f713d0ffebe in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:574:9
    #22 0x7f713d9a61af in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) src/layout/ipc/VsyncChild.cpp:68:16
    #23 0x7f713662ee3d in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:167:20
    #24 0x7f71364f352d in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1988:28
    #25 0x7f713600998e in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2136:25
    #26 0x7f71360068d2 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2066:17
    #27 0x7f713600810c in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1912:5
    #28 0x7f7136008768 in mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1945:15
    #29 0x7f71351124f6 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1088:14
    #30 0x7f713512e430 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #31 0x7f713601162a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #32 0x7f7135f64cd9 in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #33 0x7f7135f64cd9 in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #34 0x7f7135f64cd9 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #35 0x7f713cb9a88a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
    #36 0x7f7140dfe2cb in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22
    #37 0x7f7135f64cd9 in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #38 0x7f7135f64cd9 in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #39 0x7f7135f64cd9 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #40 0x7f7140dfdc90 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34
    #41 0x4f50dc in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #42 0x4f50dc in main src/browser/app/nsBrowserApp.cpp:282
    #43 0x7f7154aa182f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #44 0x42476c in _start (firefox+0x42476c)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Comment 2

[Matt Woodrow (:mattwoodrow)]

I'm not able to reproduce this on OSX.

Will try again on linux soon.

Do you know what the MOZ_CrashPrintf error message was?

Comment 3

[Matt Woodrow (:mattwoodrow)]

No luck on linux with mozilla-central 6b9076ac236c.

Any other ideas to get this to reproduce?

Comment 4

[Tyson Smith [:tsmith]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> No luck on linux with mozilla-central 6b9076ac236c.
> 
> Any other ideas to get this to reproduce?

Yeah it seems it needs to be served from a http server, not sure why.

I threw the test in it's own sub directory and run this cmd from the subdir:
"python -m SimpleHTTPServer" or "python3 -m http.server"
then opened http://localhost:8000/testcase.html

Let me know if that doesn't work, sorry about that.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1464641 - Make sure we mark the out of flow frame as modified when we modify the placeholder.

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

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Tyson Smith [:tsmith] from comment #4)
> 
> Yeah it seems it needs to be served from a http server, not sure why.
> 
> I threw the test in it's own sub directory and run this cmd from the subdir:
> "python -m SimpleHTTPServer" or "python3 -m http.server"
> then opened http://localhost:8000/testcase.html
> 
> Let me know if that doesn't work, sorry about that.

Still nothing on OSX, but reproduced perfectly on Linux. I have no explanations, actual bug is platform-independent and has nothing to do with the network. Thanks for your help, these reduced test cases are amazing!

The actual bug happens when we reflow and move a placeholder frame to a different line.

We call nsIFrame::SetParent for the new line, and that calls SchedulePaint -> MarkNeedsDisplayItemRebuild.

This function does an early return since we don't want to deal with placeholders in the modified frames list. I believe the logic here was that we always invalidate both the OOF and the placeholder, so we only need to record one of them. This is not true for this case (but is true for style changes), so the fix is to just make sure we explicitly pass the invalidation on to the OOF.

Performance shouldn't be affected, since we also have an early return for already modified frame, so the extra call (for the cases where we already had the OOF modified) should return quickly.

Comment 7

[Miko Mynttinen]

Comment on attachment 8981263 [details]
Bug 1464641 - Make sure we mark the out of flow frame as modified when we modify the placeholder.

https://reviewboard.mozilla.org/r/247356/#review253590

LGTM.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df864b81b63a
Make sure we mark the out of flow frame as modified when we modify the placeholder. r=miko

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/df864b81b63a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 11

[Miko Mynttinen]

Comment on attachment 8981263 [details]
Bug 1464641 - Make sure we mark the out of flow frame as modified when we modify the placeholder.

Approval Request Comment
[Feature/Bug causing the regression]: Display item was not invalidated when the underlying frame was modified. Caused invalid merging of retained display list.
[User impact if declined]: Crashes on Nightly/Beta with assertions. Undefined behavior, possibly invalid merging, on other channels.
[Is this code covered by automated tests?]: Crashtest available, not included in the patch.
[Has the fix been verified in Nightly?]: Yes, cannot reproduce on Mac or Linux.
[Needs manual test from QE? If yes, steps to reproduce]: No. STR: open the attached testcase served from an HTML server.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: This patch causes us to rebuild more display items, which slightly increase the risk of merging failure, but also reduces the chance of reused old items ending up in the wrong list.
[String changes made/needed]: No.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Turns out the attached testcase reproduces nicely in CI when served over HTTP too (and not on revs from after this landed). I'm going to land it.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c736d73f5d9
Add crashtest.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8981263 [details]
Bug 1464641 - Make sure we mark the out of flow frame as modified when we modify the placeholder.

RDL crash fix with a new crashtest. Approved for 61.0b10.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a56dbd423630
https://hg.mozilla.org/releases/mozilla-beta/rev/13924a524f1c

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1c736d73f5d9