Closed Bug 1468124 Opened 6 years ago Closed 6 years ago

crash near null in [@ TransformGfxRectToAncestor]

Categories

(Core :: Web Painting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- disabled
firefox60 --- disabled
firefox61 - fixed
firefox62 + fixed

People

(Reporter: tsmith, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file testcase.html (deleted) —
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)
Flags: in-testsuite?
I think this is a retained display list bug.  The test case in comment 0 doesn't crash with disabling retained display list.
Blocks: RDLbugs
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;

?
Blocks: 1419225
(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;
Component: Layout → Layout: Web Painting
(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|.
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
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
Attachment #8985286 - Flags: review?(mikokm) → 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
Attachment #8985287 - Flags: review?(mikokm) → review-
(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
(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.
Attachment #8985287 - Attachment is obsolete: true
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.
Attachment #8985286 - Flags: review+
Thank you Matt for reviewing!

This try push will be a final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1357f080fe90dff22bd7078403bd648e69836738
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
https://hg.mozilla.org/mozilla-central/rev/245c6fe9938c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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.
Attachment #8985286 - Flags: approval-mozilla-beta?
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.
Attachment #8985286 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: