Closed Bug 1256045 Opened 4 years ago Closed 4 years ago

crash in mozilla::layers::BufferTextureHost::EnsureWrappingTextureSource

Categories

(Core :: Graphics: Layers, defect, critical)

47 Branch
All
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mats, Assigned: nical)

Details

(Keywords: crash, regression, Whiteboard: gfx-noted)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-246ef209-f3a1-4b34-9960-f42ea2160311.
=============================================================

Very few crashes so far, but only in v47 and v48 so it's likely to become worse.

Operating System 	Percentage 	Number Of Crashes
Windows 10 	68.75% 	11
Windows 7 	18.75% 	3
Windows 8.1 	12.50% 	2

Product 	Version 	Percentage 	Number Of Crashes
Firefox 	47.0a1 	56.25% 	9
Firefox 	48.0a1 	43.75% 	7

It looks like a null-pointer crash:
 x86 	EXCEPTION_ACCESS_VIOLATION_WRITE 	0x14
amd64 	EXCEPTION_ACCESS_VIOLATION_WRITE 	0x28 

Stack:
mozilla::layers::BufferTextureHost::EnsureWrappingTextureSource()
mozilla::layers::BufferTextureHost::Upload(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits>*)
mozilla::layers::BufferTextureHost::MaybeUpload(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits>*)
mozilla::layers::BufferTextureHost::UpdatedInternal(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const*)
mozilla::layers::ImageHost::UseTextureHost(nsTArray<mozilla::layers::CompositableHost::TimedTexture> const&)
mozilla::layers::CompositableParentManager::ReceiveCompositableUpdate(mozilla::layers::CompositableOperation const&, std::vector<mozilla::layers::EditReply, std::allocator<mozilla::layers::EditReply> >&)
mozilla::layers::LayerTransactionParent::RecvUpdate(nsTArray<mozilla::layers::Edit>&&, nsTArray<mozilla::layers::OpDestroy>&&, unsigned __int64 const&, mozilla::layers::TargetConfig const&, nsTArray<mozilla::layers::PluginWindowData>&&, bool const&, bool const&, unsigned int const&, bool const&, mozilla::TimeStamp const&, int const&, nsTArray<mozilla::layers::EditReply>*)
mozilla::layers::PLayerTransactionParent::OnMessageReceived(IPC::Message const&, IPC::Message*&)
mozilla::layers::PCompositorParent::OnMessageReceived(IPC::Message const&, IPC::Message*&)
mozilla::ipc::MessageChannel::DispatchSyncMessage(IPC::Message const&, IPC::Message*&)
mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&)
mozilla::ipc::MessageChannel::OnMaybeDequeueOne()
RunnableMethod<SoftwareDisplay, void ( SoftwareDisplay::*)(void), mozilla::Tuple<> >::Run()
MessageLoop::DoWork()
base::MessagePumpForUI::DoRunLoop()
base::MessagePumpWin::Run(base::MessagePump::Delegate*)
MessageLoop::RunHandler()
MessageLoop::Run()
base::Thread::ThreadMain()
[...]
Nical, looks like we don't guard against mCompositor->CreateDataTextureSourceAround() returning nullptr in BufferTextureHost::EnsureWrappingTextureSource() ?
Whiteboard: gfx-noted
(In reply to Jamie Nicol [:jnicol] from comment #1)
> Nical, looks like we don't guard against
> mCompositor->CreateDataTextureSourceAround() returning nullptr in
> BufferTextureHost::EnsureWrappingTextureSource() ?

We should guard against it to be safe, but we should not be getting into this code path in the first place. Here we have a texture that is set to not have an intermediate buffer which should only happen when using the BasicCompositor, in which case CreateDataTextureSourceAround never returns null.
I'll have a look.
Assignee: nobody → nical.bugzilla
Attached patch Null checkSplinter Review
I'll continue investigating why we get here in another bug. I suspect that the texture switched from a basic to a non-basic compositor which might be causing issues in other places as well.
Attachment #8731620 - Flags: review?(jnicol)
Comment on attachment 8731620 [details] [diff] [review]
Null check

Review of attachment 8731620 [details] [diff] [review]:
-----------------------------------------------------------------

If this really shouldn't be happening is it worth asserting in debug builds?
Attachment #8731620 - Flags: review?(jnicol) → review+
(In reply to Jamie Nicol [:jnicol] from comment #4)
> If this really shouldn't be happening is it worth asserting in debug builds?

I am hesitant to assert here because there's another bug in which I've added assertions around similar symptoms where asserting give us a more helpful stack. I just added a comment and a warning to this patch instead.
okay, sounds good
https://hg.mozilla.org/mozilla-central/rev/7960c66bd189
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
would this fix be upliftable to 47? 
it seems to have worked well, as there were no more crashes on central after the patch landed and through the whole aurora cycle so far (and it would be nice, if we don't ship the regression with 47).
Flags: needinfo?(nical.bugzilla)
Keywords: regression
Comment on attachment 8731620 [details] [diff] [review]
Null check

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Some crashes.
[Describe test coverage new/current, TreeHerder]: The patch has been baking in central and aurora for a while without issues.
[Risks and why]: None. Trivial null-check. The added branch is only taken in situations where we are crashing without patch.
[String/UUID change made/needed]: None.
Flags: needinfo?(nical.bugzilla)
Attachment #8731620 - Flags: approval-mozilla-beta?
Comment on attachment 8731620 [details] [diff] [review]
Null check

Not-null check, there are ~300 total crashes on this on Beta47 for a week, stabilized on aurora and nightly for a while. let's take it.
Attachment #8731620 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.