1402157 - stylo: Assertion failure: aFrame->HasImageRequest() (why call me?) in [@ mozilla::css::ImageLoader::DisassociateRequestFromFrame]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Tyson Smith [:tsmith] (PTO)]

Attachment: test_case.html

Assertion failure: aFrame->HasImageRequest() (why call me?), at /builds/worker/workspace/build/src/layout/style/ImageLoader.cpp:183

#0 0x7f4141115bd2 in mozilla::css::ImageLoader::DisassociateRequestFromFrame(imgIRequest*, nsIFrame*) /src/layout/style/ImageLoader.cpp:183:3
#1 0x7f414162959f in std::_Function_handler<void (imgRequestProxy*), AddAndRemoveImageAssociations(nsFrame*, nsStyleImageLayers const*, nsStyleImageLayers const*)::$_7>::_M_invoke(std::_Any_data const&, imgRequestProxy*) /src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/functional:2039:2
#2 0x7f414162941c in std::function<void (imgRequestProxy*)>::operator()(imgRequestProxy*) const /src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/functional:2440:14
#3 0x7f4141628f78 in CompareLayers(nsStyleImageLayers const*, nsStyleImageLayers const*, std::function<void (imgRequestProxy*)> const&) /src/layout/generic/nsFrame.cpp:854:9
#4 0x7f41415e53fb in AddAndRemoveImageAssociations(nsFrame*, nsStyleImageLayers const*, nsStyleImageLayers const*) /src/layout/generic/nsFrame.cpp:877:5
#5 0x7f414159fb5b in nsFrame::DidSetStyleContext(nsStyleContext*) /src/layout/generic/nsFrame.cpp:922:3
#6 0x7f41413a2450 in nsIFrame::SetStyleContext(nsStyleContext*) /src/layout/generic/nsIFrame.h:770:7
#7 0x7f41416c1842 in nsInlineFrame::UpdateStyleOfOwnedAnonBoxesForIBSplit(mozilla::ServoRestyleState&) /src/layout/generic/nsInlineFrame.cpp:1061:13
#8 0x7f4141610abb in nsIFrame::DoUpdateStyleOfOwnedAnonBoxes(mozilla::ServoRestyleState&) /src/layout/generic/nsFrame.cpp:10651:42
#9 0x7f41414090fb in mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:877:19
#10 0x7f414140934d in mozilla::ServoRestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoStyleContext*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:920:32
#11 0x7f414140bb2c in mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:1121:28
#12 0x7f41413d736c in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4170:41
#13 0x7f414136bf73 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1921:18
#14 0x7f414137548e in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /src/layout/base/nsRefreshDriver.cpp:307:7
#15 0x7f4141375264 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:328:5
#16 0x7f4141378855 in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:770:5
#17 0x7f41413778f6 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:683:35
#18 0x7f4141373867 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /src/layout/base/nsRefreshDriver.cpp:529:20
#19 0x7f413b9e471f in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
#20 0x7f413b9e9a90 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:521:10
#21 0x7f413c55a865 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#22 0x7f413c4af7a7 in MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#23 0x7f413c4af639 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#24 0x7f4140e6b85a in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#25 0x7f4144086641 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
#26 0x7f41441eab01 in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22
#27 0x7f41441ec7ca in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8
#28 0x7f41441ed6b8 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21
#29 0x4ed398 in do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:236:22
#30 0x4eccb0 in main /src/browser/app/nsBrowserApp.cpp:309:16
#31 0x7f415ab4282f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#32 0x41e9e4 in _start (firefox+0x41e9e4)

Comment 1

[Mats Palmgren (inactive)]

I can't reproduce it in any of my local Linux64 debug builds.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Reproduces on Win10 for me, but not on Ubuntu 17.04. And only with Stylo enabled.

INFO: Last good revision: 8b274bfe790511b3517f9dc98f54f56efc8dee7c
INFO: First bad revision: 319bc66e5d3a6599a61b686bc58ee399dac77b65
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8b274bfe790511b3517f9dc98f54f56efc8dee7c&tochange=319bc66e5d3a6599a61b686bc58ee399dac77b65

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

I can reproduce it with my local windows debug build, and there seems to be another bug that both processes can be hanged with a reduced testcase.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> I can reproduce it with my local windows debug build, and there seems to be
> another bug that both processes can be hanged with a reduced testcase.

That one is because we repeatedly call load() in onerror for an invalid url, which is basically a infinite loop, so probably not a bug.

But this does help me understand this testcase.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: simplified testcase

A somehow simplified testcase.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: simplified testcase

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Given this testcase, I suppose that it is related to resolution of fragment url with location change.

Not sure what's happening here. CJKu may know more details.

Comment 8

[Astley Chen (inactive)]

Assigned to CJ to figure out.

Comment 9

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

We might want to track this for 57 because this is a reproducible process hang.

Comment 10

[Marcia Knous [:marcia]]

(In reply to Chris Peterson [:cpeterson] from comment #9)
> [Tracking Requested - why for this release]:
> 
> We might want to track this for 57 because this is a reproducible process
> hang.

Tracking 57+ for this issue.

Comment 11

[u459114]

Attachment: Test.html

Comment 12

[u459114]

Attachment: Bug 1402157 - Disassociate a frame with an image loader if that frame indeed has any image request.

Before enable stylo, an image request is always been resolved before the
creation time of that request object.
As a result, in AddAndRemoveImageAssociations, we always associate the request
objects of style images with the given frame(since they are resolved).

After enable stylo, an image request is resolved after been created. It is
possible that
1. AddAndRemoveImageAssociations is called. The image request is not resolved
   yet, so we do not associate that frame with that image request.
2. That image request is been resolved via nsStyleXXXX::FinishStyle.
3. Style changed. AddAndRemoveImageAssociations is called again. We try to
   dis-associate the frame with image request, then hit this assertion since
   we did not assicate this frame with any image request in #1.

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

Comment 13

[u459114]

Attachment: Bug 1402157 - Part 2. A crash test for disassociating image loaders and frames.

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

Comment 14

[u459114]

Comment on attachment 8918189 [details]
Bug 1402157 - Disassociate a frame with an image loader if that frame indeed has any image request.

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

Comment 15

[u459114]

Comment on attachment 8918189 [details]
Bug 1402157 - Disassociate a frame with an image loader if that frame indeed has any image request.

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

Comment 16

[u459114]

Comment on attachment 8918212 [details]
Bug 1402157 - Part 2. A crash test for disassociating image loaders and frames.

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

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8918189 [details]
Bug 1402157 - Disassociate a frame with an image loader if that frame indeed has any image request.

https://reviewboard.mozilla.org/r/189068/#review195236

Sorry for the delay.

::: commit-message-2ca37:8
(Diff revision 3)
> +Before enable stylo, an image request is always been resolved before the
> +creation time of that request object[1].
> +As a result, in AddAndRemoveImageAssociations, we always associate the request
> +objects of style images with the given frame(since they are resolved).
> +
> +After enable stylo, an image request is resolved after tha object been created[2].

s/tha/that/

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8918212 [details]
Bug 1402157 - Part 2. A crash test for disassociating image loaders and frames.

https://reviewboard.mozilla.org/r/189090/#review195240

::: commit-message-75994:1
(Diff revision 2)
> +Bug 1402157 - Part 2. A crash test for disassociate image loaders and frames.

s/disassociate/disassociating/

Comment 19

[u459114]

Comment on attachment 8918189 [details]
Bug 1402157 - Disassociate a frame with an image loader if that frame indeed has any image request.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189068/diff/3-4/

Comment 20

[u459114]

Comment on attachment 8918212 [details]
Bug 1402157 - Part 2. A crash test for disassociating image loaders and frames.

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

Comment 21

[u459114]

Attachment: Bug 1402157 - Test

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

Comment 22

[u459114]

(In reply to Marcia Knous [:marcia - use ni. PTO 10/18-10/20] from comment #10)
> (In reply to Chris Peterson [:cpeterson] from comment #9)
> > [Tracking Requested - why for this release]:
> > 
> > We might want to track this for 57 because this is a reproducible process
> > hang.
> 
> Tracking 57+ for this issue.
It's not a process hang, it's an assertion failure. Browser can still work correctly in release version. I don't think we need to uplift these patches to 57.

Comment 23

[u459114]

Comment on attachment 8919821 [details]
Bug 1402157 - Test

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

Comment 24

[u459114]

Comment on attachment 8919821 [details]
Bug 1402157 - Test

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

Comment 25

[u459114]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14969c3ddcffbfc636be9c49c39a620160846f5f
Bug 1402157 - Disassociate a frame with an image loader if that frame indeed has any image request. r=heycam

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/14969c3ddcff

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Did we not land the tests on purpose?

Comment 28

[u459114]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)
> Did we not land the tests on purpose?
Yes, that test case can not be loaded correctly in reftest harness.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

What about the crashtest?