1191532 - Crash spike on yahoo.com in mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&,

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-8d00ab04-ee30-4243-8ab3-7955b2150731.
=============================================================

According to the explosiveness reports, this signature shot up dramatically yesterday. https://crash-analysis.mozilla.com/rkaiser/2015-08-04/2015-08-04.firefox.39.explosiveness.html

User comments and URLs all point to yahoo.com. Perhaps they pushed out an update yesterday?

0 	xul.dll 	mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4 const*, unsigned int) 	layout/base/FrameLayerBuilder.cpp
1 	xul.dll 	nsDisplaySVGEffects::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) 	layout/base/nsDisplayList.cpp
2 	xul.dll 	mozilla::FrameLayerBuilder::AddPaintedDisplayItem(mozilla::PaintedLayerData*, nsDisplayItem*, mozilla::DisplayItemClip const&, mozilla::ContainerState&, mozilla::LayerState, nsPoint const&) 	layout/base/FrameLayerBuilder.cpp
3 	xul.dll 	mozilla::ContainerState::PopPaintedLayerData() 	layout/base/FrameLayerBuilder.cpp
4 	xul.dll 	mozilla::ContainerState::Finish(unsigned int*, mozilla::LayerManagerData*, nsIntRect const&, nsDisplayList*, bool&) 	layout/base/FrameLayerBuilder.cpp
5 	xul.dll 	mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4 const*, unsigned int) 	layout/base/FrameLayerBuilder.cpp

Comment 1

[(Away)]

Adding the other exploding signatures as they also appear to be correlated with Yahoo URLs.

Comment 2

[(Away)]

Kev, perhaps this may interest you?

Comment 3

[(Away)]

I should clarify that these are on the Yahoo front page, not on search results.

Comment 4

[Kev Needham [:kev]]

Yup, submitted to Yahoo product team and asked about yahoo.com vs. SERPs.

Comment 5

[Benjamin Smedberg]

Is nsDisplaySVGEffects always the second frame? This looks like it could be SVG-specific.

Comment 6

[Kev Needham [:kev]]

Yahoo's happy to investigate further with us; is there someone who can take point on helping identify/troubleshoot the root cause?

Comment 7

[(Away)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Is nsDisplaySVGEffects always the second frame? This looks like it could be SVG-specific.

Yes, that appears to be the case.

roc: aManager->GetLayerBuilder() is null at nsDisplaySVGEffects::BuildLayer. Any idea what could cause that?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That should never happen... FrameLayerBuilder::AddPaintedDisplayItem sets the FrameLayerBuilder on tempManager via FrameLayerBuilder::Init called here:

      FrameLayerBuilder* layerBuilder = new FrameLayerBuilder();
      layerBuilder->Init(mDisplayListBuilder, tempManager, aLayerData);

It's only a small amount of code from there to where we call aItem->BuildLayer:

      tempManager->BeginTransaction();
      if (mRetainingManager) {
        layerBuilder->DidBeginRetainedLayerTransaction(tempManager);
      }

      UniquePtr<LayerProperties> props(LayerProperties::CloneFrom(tempManager->GetRoot()));
      nsRefPtr<Layer> tmpLayer =
        aItem->BuildLayer(mDisplayListBuilder, tempManager, ContainerLayerParameters());

I do not see how the gLayerManagerLayerBuilder user-data on tempManager could have been cleared along that path. The body of nsDisplaySVGEffects::BuildLayer also looks innocuous.

How much data do you have in the crash dump? Does tempManager still look like a valid LayerManager in general?

Can we add a patch on some branch with assertions to narrow this down? If so then we can sprinkle release-asserts along the path in FrameLayerBuilder::AddPaintedDisplayItem asserting that the layer manager's GetLayerBuilder is still valid (or at least not null). Inside nsDisplaySVGEffects::BuildLayer we can assert !aManager->IsInactiveLayerManager() || aManager->GetLayerBuilder().

Comment 9

[(Away)]

After reading those codepaths I agree that this looks highly questionable. But I went through the disassembly again and |this| is definitely null in BuildContainerLayerFor.

The crash reports don't have heap contents so I don't know anything about tempManager. At this point I support the idea of diagnostic patches.

Comment 10

[Jonathan Watt [:jwatt]]

Kev, needinfo'ing you about following up with Yahoo about whether the subsequent drop in these crashes may have been due to a change they made.

Comment 11

[Tobias B. Besemer [:BesTo] (QA)]

Not sure, if it is correlated ...
... but pretty sure, that I wasn't on Yahoo ...

[@ mozilla::layers::LayerManagerUserDataDestroy ]

https://crash-stats.mozilla.com/report/index/2b5a59b0-fdcd-4e4d-b149-f001b2150909

Crashing Thread:
Frame 	Module 	Signature				 	Source
0 	xul.dll mozilla::layers::LayerManagerUserDataDestroy 	gfx/layers/Layers.h
1 		@0x6e6f6974616368

Comment 12

[Kev Needham [:kev]]

Closing this out. Yahoo did make changes because of the reported issue. Sorry I never cleaned this out.