Last Comment Bug 690469 - ShadowThebesLayer Init/Swap API rework
: ShadowThebesLayer Init/Swap API rework
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla10
Assigned To: Oleg Romashin (:romaxa)
: Milan Sreckovic [:milan]
Depends on: 693086 693282 694140 694964
Blocks: 691417
  Show dependency treegraph
Reported: 2011-09-29 12:05 PDT by Oleg Romashin (:romaxa)
Modified: 2012-02-05 13:27 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

ShadowThebesLayer Init/Swap API rework (31.87 KB, patch)
2011-09-29 12:06 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
Details | Diff | Splinter Review
ShadowThebesLayer Init/Swap API rework (32.83 KB, patch)
2011-10-03 09:00 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
ShadowThebesLayer Init/Swap API rework (32.87 KB, patch)
2011-10-04 00:18 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2011-09-29 12:05:05 PDT
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) 2011-09-29 12:06:54 PDT
Created attachment 563505 [details] [diff] [review]
ShadowThebesLayer Init/Swap API rework
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-30 17:34:45 PDT
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

>+  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) 2011-10-03 09:00:11 PDT
Created attachment 564207 [details] [diff] [review]
ShadowThebesLayer Init/Swap API rework

Here is updated patch, but I found some reftest failures on try server...

will check what else is wrong here.
Comment 4 Oleg Romashin (:romaxa) 2011-10-03 10:48:58 PDT
> >+  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) 2011-10-03 13:59:55 PDT
try build
Comment 6 Oleg Romashin (:romaxa) 2011-10-04 00:18:21 PDT
Created attachment 564474 [details] [diff] [review]
ShadowThebesLayer Init/Swap API rework
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 2011-10-04 11:18:51 PDT
Ah yes, you're right.  IPDL unions don't get a default value.  My fault.
Comment 9 Ed Morley [:emorley] 2011-10-07 04:58:01 PDT
Comment 10 Gian-Carlo Pascutto [:gcp] 2011-10-10 07:57:19 PDT
See bug 693086. Causes very frequent crashes of the content process on Fennec.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2011-10-10 08:04:50 PDT
backed out due to crashes reported in bug 693086
Comment 12 Matt Brubeck (:mbrubeck) 2011-10-10 09:59:16 PDT
The backout was backed out until we can figure out which of several patches broke Android tests:

Leaving this bug open, as we still plan to back it out ASAP.
Comment 13 Matt Brubeck (:mbrubeck) 2011-10-10 15:23:29 PDT
Re-landed the backout (with a clobber to prevent the test failures):
Comment 14 Oleg Romashin (:romaxa) 2011-10-10 16:14:32 PDT
This patch need to be landed in couple with patch from bug 690469
in order 
1-st bug 690469, second 693282
Comment 15 Matt Brubeck (:mbrubeck) 2011-10-10 16:46:02 PDT
This was re-landed on inbound:
Comment 16 Matt Brubeck (:mbrubeck) 2011-10-11 14:32:57 PDT

Note You need to log in before you can comment on or make changes to this bug.