Tyson Smith [:tsmith] Reporter
	Description • 6 years ago

This testcase seems a bit fussy but does reproduce the issue reliably on a m-c ASan-opt build.

==93709==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x7fcfdfb08dfd bp 0x7ffe94d3ee40 sp 0x7ffe94d3eb40 T0)
==93709==The signal is caused by a READ memory access.
==93709==Hint: address points to the zero page.
    #0 0x7fcfdfb08dfc in get src/obj-firefox/dist/include/mozilla/RefPtr.h:296:27
    #1 0x7fcfdfb08dfc in operator mozilla::ComputedStyle * src/obj-firefox/dist/include/mozilla/RefPtr.h:309
    #2 0x7fcfdfb08dfc in Style src/layout/generic/nsIFrame.h:783
    #3 0x7fcfdfb08dfc in PresContext src/layout/generic/nsIFrame.h:628
    #4 0x7fcfdfb08dfc in TransformGfxRectToAncestor(nsIFrame*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, nsIFrame const*, bool*, mozilla::Maybe<mozilla::gfx::Matrix4x4TypedFlagged<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> >*, bool, nsIFrame**) src/layout/base/nsLayoutUtils.cpp:2909
    #5 0x7fcfdfb080d0 in nsLayoutUtils::TransformFrameRectToAncestor(nsIFrame*, nsRect const&, nsIFrame const*, bool*, mozilla::Maybe<mozilla::gfx::Matrix4x4TypedFlagged<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> >*, bool, nsIFrame**) src/layout/base/nsLayoutUtils.cpp:2982:14
    #6 0x7fcfe0367691 in ProcessFrameInternal(nsIFrame*, nsDisplayListBuilder&, AnimatedGeometryRoot**, nsRect&, nsIFrame*, nsTArray<nsIFrame*>&, bool) src/layout/painting/RetainedDisplayListBuilder.cpp:748:17
    #7 0x7fcfe0367e24 in ProcessFrameInternal(nsIFrame*, nsDisplayListBuilder&, AnimatedGeometryRoot**, nsRect&, nsIFrame*, nsTArray<nsIFrame*>&, bool) src/layout/painting/RetainedDisplayListBuilder.cpp:742:7
    #8 0x7fcfe0366372 in RetainedDisplayListBuilder::ProcessFrame(nsIFrame*, nsDisplayListBuilder&, nsIFrame*, nsTArray<nsIFrame*>&, bool, nsRect*, AnimatedGeometryRoot**) src/layout/painting/RetainedDisplayListBuilder.cpp:899:3
    #9 0x7fcfe036a117 in RetainedDisplayListBuilder::ComputeRebuildRegion(nsTArray<nsIFrame*>&, nsRect*, AnimatedGeometryRoot**, nsTArray<nsIFrame*>&) src/layout/painting/RetainedDisplayListBuilder.cpp:997:10
    #10 0x7fcfe036b8bc in RetainedDisplayListBuilder::AttemptPartialUpdate(unsigned int, mozilla::DisplayListChecker*) src/layout/painting/RetainedDisplayListBuilder.cpp:1141:8
    #11 0x7fcfdfb1377a in nsLayoutUtils::PaintFrame(gfxContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) src/layout/base/nsLayoutUtils.cpp:3695:40
    #12 0x7fcfdfa06dda in mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) src/layout/base/PresShell.cpp:6314:5
    #13 0x7fcfdf393067 in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) src/view/nsViewManager.cpp:480:19
    #14 0x7fcfdf391e7c in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) src/view/nsViewManager.cpp:412:33
    #15 0x7fcfdf3974a6 in nsViewManager::ProcessPendingUpdates() src/view/nsViewManager.cpp:1102:5
    #16 0x7fcfdf9800ba in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:2039:11
    #17 0x7fcfdf98cccb in TickDriver src/layout/base/nsRefreshDriver.cpp:328:13
    #18 0x7fcfdf98cccb in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:301
    #19 0x7fcfdf98c899 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:320:5
    #20 0x7fcfdf98f3de in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:760:5
    #21 0x7fcfdf98f3de in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:673
    #22 0x7fcfdf98efec in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:574:9
    #23 0x7fcfe024515f in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) src/layout/ipc/VsyncChild.cpp:68:16
    #24 0x7fcfd8f0c908 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:167:20
    #25 0x7fcfd8de3ff7 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1988:28
    #26 0x7fcfd894f8ce in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2134:25
    #27 0x7fcfd894c7e4 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2064:17
    #28 0x7fcfd894e03c in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1910:5
    #29 0x7fcfd894e698 in mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1943:15
    #30 0x7fcfd7a5f8fa in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1088:14
    #31 0x7fcfd7a7bb54 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #32 0x7fcfd8957576 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:125:5
    #33 0x7fcfd88ac00c in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #34 0x7fcfd88ac00c in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #35 0x7fcfd88ac00c in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #36 0x7fcfdf4215ea in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
    #37 0x7fcfe36c8e8f in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:896:22
    #38 0x7fcfd88ac00c in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #39 0x7fcfd88ac00c in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #40 0x7fcfd88ac00c in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #41 0x7fcfe36c8846 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:722:34
    #42 0x4f1ca4 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #43 0x4f1ca4 in main src/browser/app/nsBrowserApp.cpp:287
    #44 0x7fcff73b982f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #45 0x4210e8 in _start (firefox+0x4210e8)

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 1 • 6 years ago

I think this is a retained display list bug.  The test case in comment 0 doesn't crash with disabling retained display list.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 2 • 6 years ago

The code in TransformGfxRectToAncestor was introduced in bug 1419225.

I wonder why we don't simply use |aFrame|'s pres context there.  Does the app units in device pixel differ from documents?  That's said, 

 const nsIFrame* ancestor = aOutAncestor ? *aOutAncestor : aAncestor;

Does this just do

 const nsIFrame* ancestor = *aOutAncestor ? *aOutAncestor : aAncestor;

?

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 3 • 6 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> The code in TransformGfxRectToAncestor was introduced in bug 1419225.
> 
> I wonder why we don't simply use |aFrame|'s pres context there.  Does the
> app units in device pixel differ from documents?  That's said, 
> 
>  const nsIFrame* ancestor = aOutAncestor ? *aOutAncestor : aAncestor;
> 
> Does this just do
> 
>  const nsIFrame* ancestor = *aOutAncestor ? *aOutAncestor : aAncestor;
> 
> ?

No, maybe;

  const nsIFrame* ancestor = aOutAncestor && *aOutAncestor ? *aOutAncestor : aAncestor;

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 4 • 6 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > The code in TransformGfxRectToAncestor was introduced in bug 1419225.
> > 
> > I wonder why we don't simply use |aFrame|'s pres context there.  Does the
> > app units in device pixel differ from documents?  That's said, 
> > 
> >  const nsIFrame* ancestor = aOutAncestor ? *aOutAncestor : aAncestor;
> > 
> > Does this just do
> > 
> >  const nsIFrame* ancestor = *aOutAncestor ? *aOutAncestor : aAncestor;
> > 
> > ?
> 
> No, maybe;
> 
>   const nsIFrame* ancestor = aOutAncestor && *aOutAncestor ? *aOutAncestor :
> aAncestor;

The real problem is that when we called HandlePreserve3D() in ProcessFrameInternal, we walked up the frame tree beyond |aStopAtFrame|, so when we called TransformGfxRectToAncestor() |aAncestor| was not an ancestor any more, it's actually a descendant of |aFrame|.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 5 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3edd33111c1b3103601670a4ef8520aae5f97228

	Miko Mynttinen
	Comment 8 • 6 years ago

I spoke with Matt about this, and the conclusion was that this function should simply go away for now, and if we encounter a modified frame with a 3D transform, the display list retaining should be disabled. This simplifies things, and it is what we would do at the later display list builds anyway [1].

To put it in a different way: Hiro's patches are correct, but there are other problems with retaining display lists with 3D transforms that should be solved later.

[1]: https://searchfox.org/mozilla-central/rev/40577076a6e7a14d143725d0288353c250ea2b16/layout/generic/nsFrame.cpp#2864

	Miko Mynttinen
	Comment 9 • 6 years ago
mozreview-review

Comment on attachment 8985286 [details]
Bug 1468124 - Don't calculate overflow area if there is any frames in preserve-3d context.

https://reviewboard.mozilla.org/r/250900/#review257282

	Miko Mynttinen
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8985287 [details]
Bug 1468124 - Use GetInFlowParent instead of GetParent to find the root node of the preserve-3d.

https://reviewboard.mozilla.org/r/250902/#review257298

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 11 • 6 years ago

(In reply to Miko Mynttinen [:miko] from comment #8)
> I spoke with Matt about this, and the conclusion was that this function
> should simply go away for now, and if we encounter a modified frame with a
> 3D transform, the display list retaining should be disabled. This simplifies
> things, and it is what we would do at the later display list builds anyway
> [1].

Thanks Miko and Matt.  I didn't notice that.  Dropping the function for now makes sense.

I am still not sure we should bail out from whole retained list stuff in such cases, but just bail out there and disable partial display list update works fine in the case where there is any frames in preserve-3d context on a try[1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d64b467165672ce4a6928838e703ddf29eaf3a

	Miko Mynttinen
	Comment 12 • 6 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> (In reply to Miko Mynttinen [:miko] from comment #8)
> > I spoke with Matt about this, and the conclusion was that this function
> > should simply go away for now, and if we encounter a modified frame with a
> > 3D transform, the display list retaining should be disabled. This simplifies
> > things, and it is what we would do at the later display list builds anyway
> > [1].
> 
> Thanks Miko and Matt.  I didn't notice that.  Dropping the function for now
> makes sense.
> 
> I am still not sure we should bail out from whole retained list stuff in
> such cases, but just bail out there and disable partial display list update
> works fine in the case where there is any frames in preserve-3d context on a
> try[1].
I think that the problem with this approach is that this flag is only read during the next display list build.

With this testcase, we add preserve-3d frame using JavaScript. To properly build a changed display list, we need to calculate the invalidated area of this new frame, so that we can rebuild display items for frames that intersect with preserve-3d context root. This is a lot of extra complexity just to retain display list for one additional paint.

> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d64b467165672ce4a6928838e703ddf29eaf3a
This patch looks great. I think that instead of using SetDisablePartialUpdates(), it might be faster to change the return type of ProcessFrameInternal to bool, and to do an early exit and propagate false values up to ProcessFrame()[1,2] and ComputeRebuildRegion()[3].

[1]: https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/layout/painting/RetainedDisplayListBuilder.cpp#742
[2]: https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/layout/painting/RetainedDisplayListBuilder.cpp#899
[3]: https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/layout/painting/RetainedDisplayListBuilder.cpp#1014

Thank you for working on this Hiro.

	Matt Woodrow (:mattwoodrow)
	Comment 14 • 6 years ago
mozreview-review

Comment on attachment 8985286 [details]
Bug 1468124 - Don't calculate overflow area if there is any frames in preserve-3d context.

https://reviewboard.mozilla.org/r/250900/#review257822

::: layout/painting/RetainedDisplayListBuilder.cpp:682
(Diff revision 2)
> -  if (last != aFrame) {
> -    aOverflow = last->GetVisualOverflowRectRelativeToParent();
> -    CRR_LOG("HandlePreserve3D() Updated overflow rect to: %d %d %d %d\n",
> -             aOverflow.x, aOverflow.y, aOverflow.width, aOverflow.height);
> -  }
> +    }
> -
> +    currentFrame = currentFrame->GetInFlowParent();

I don't think we actually need to walk up the frame tree here, as TransformFrameRectToAncestor does this, and returns on every stacking context in the ancestor chain.

We might need to check for IsInPreserve3DContext after the TransformFrameRectToAncestor call though.

r=me with that.

	Hiroyuki Ikezoe (:hiro) Assignee
	Comment 16 • 6 years ago

Thank you Matt for reviewing!

This try push will be a final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1357f080fe90dff22bd7078403bd648e69836738

	Pulsebot
	Comment 17 • 6 years ago

Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/245c6fe9938c
Don't calculate overflow area if there is any frames in preserve-3d context. r=mattwoodrow

	Dorel Luca [:dluca]
	Comment 18 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/245c6fe9938c

	Matt Woodrow (:mattwoodrow)
	Comment 19 • 6 years ago

Comment on attachment 8985286 [details]
Bug 1468124 - Don't calculate overflow area if there is any frames in preserve-3d context.

Approval Request Comment
[Feature/Bug causing the regression]: RDL.
[User impact if declined]: Seems to be rare, but seems like it could happen for real content on the web. Not a debug assertion or anything, so will affect release.
[Is this code covered by automated tests?]: New crashtest added.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just bails out from RDL partial updates when it detects preserve-3d, falls back to the non-RDL code path. We have existing bail outs too, so very little new here.
[String changes made/needed]: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago

Comment on attachment 8985286 [details]
Bug 1468124 - Don't calculate overflow area if there is any frames in preserve-3d context.

Disables RDL in some preserve-3d situations to avoid crashing. Approved for 61.0rc2.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-release/rev/865f9ac1818a