690469 - ShadowThebesLayer Init/Swap API rework

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Followup for bug 689045.
Idea is to allocate buffer only when it is needed, make ShadowableThebesLayer allocate only back buffer when it is null, and ShadowThebesLayer should destroy front buffer on destroy or backBuffer and front buffer size different

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: ShadowThebesLayer Init/Swap API rework

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 563505 [details] [diff] [review]
ShadowThebesLayer Init/Swap API rework

>+  // Sync front/back buffers content
>+  virtual void SyncData() {}
>+

This name isn't very descriptive.  Please change it to something like
|SyncFrontBufferToBackBuffer)|.

>+  PRPackedBool mNeedSyncData;

Also please change this to something like |mFrontAndBackBufferDiffer|.

>   // This code relies on Swap() arriving *after* attribute mutations.
>-  aNewBack->buffer() = mFrontBufferDescriptor;
>+  if (IsSurfaceDescriptorValid(mFrontBufferDescriptor)) {
>+    *aNewBack = ThebesBuffer();

This isn't necessary.

>+    aNewBack->get_ThebesBuffer().buffer() = mFrontBufferDescriptor;
>+  } else {
>+    NS_WARNING("Front is not valid, set back buffer to null_t");

This shouldn't be a warning, it's part of normal operation when the surface size changes.

It feels to me like this patch should be simpler, but I don't have any
better ideas.

r=me with the fixes above.

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: ShadowThebesLayer Init/Swap API rework

Here is updated patch, but I found some reftest failures on try server...
https://tbpl.mozilla.org/?tree=Try&rev=63618b397502

will check what else is wrong here.

Comment 4

[Oleg Romashin (:romaxa)]

> >+  if (IsSurfaceDescriptorValid(mFrontBufferDescriptor)) {
> >+    *aNewBack = ThebesBuffer();
> 
> This isn't necessary.
actually it is, because before swap call newBack is set to OptionalThebesBuffer, and that is not initialized as ThebesBuffer...

and we will have this crash
REFTEST TEST-START | file://content/base/crashtests/340733-1.html | 18 / 98 (18%)
++DOMWINDOW == 40 (0xaf0e75a8) [serial = 40] [outer = 0xb3501a10]
###!!! ABORT: unexpected type tag: '(mType) == (aType)', file ../../ipc/ipdl/_ipdlheaders/mozilla/layers/PLayers.h, line 1194
mozilla::layers::OptionalThebesBuffer::AssertSanity(mozilla::layers::OptionalThebesBuffer::Type) const (obj-ff-gtk-ds/ipc/ipdl/../../ipc/ipdl/_ipdlheaders/mozilla/layers/PLayers.h:1194)
mozilla::layers::BasicShadowThebesLayer::Swap(mozilla::layers::ThebesBuffer const&, nsIntRegion const&, mozilla::layers::OptionalThebesBuffer*, nsIntRegion*, mozilla::layers::OptionalThebesBuffer*, nsIntRegion*) (gfx/layers/basic/BasicLayers.cpp:2788)
.L186 (gfx/layers/ipc/ShadowLayersParent.cpp:331)
mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (obj-ff-gtk-ds/ipc/ipdl/PLayersParent.cpp:220)
mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (obj-ff-gtk-ds/ipc/ipdl/PContentParent.cpp:1384)

Comment 5

[Oleg Romashin (:romaxa)]

try build 
https://tbpl.mozilla.org/?tree=Try&rev=d7cebf6100db

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: ShadowThebesLayer Init/Swap API rework

https://tbpl.mozilla.org/?tree=Try&rev=b81528ed2ba1
Ok, checked bunch of try's and it seems all oranges which are happening in my builds also available in others...
So it should be good enough to checkin

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Ah yes, you're right.  IPDL unions don't get a default value.  My fault.

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/703df62e855b

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/703df62e855b

Comment 10

[Gian-Carlo Pascutto [:gcp]]

See bug 693086. Causes very frequent crashes of the content process on Fennec.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

backed out due to crashes reported in bug 693086 https://hg.mozilla.org/mozilla-central/rev/524a6bb8744b

Comment 12

[Matt Brubeck (:mbrubeck)]

The backout was backed out until we can figure out which of several patches broke Android tests:
https://hg.mozilla.org/mozilla-central/rev/bcf12565b95b

Leaving this bug open, as we still plan to back it out ASAP.

Comment 13

[Matt Brubeck (:mbrubeck)]

Re-landed the backout (with a clobber to prevent the test failures):
https://hg.mozilla.org/mozilla-central/rev/b0e79255fa97

Comment 14

[Oleg Romashin (:romaxa)]

This patch need to be landed in couple with patch from bug 690469
in order 
1-st bug 690469, second 693282
https://tbpl.mozilla.org/?tree=Try&rev=5afde775820a

Comment 15

[Matt Brubeck (:mbrubeck)]

This was re-landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b88a4500aa

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a5b88a4500aa