Closed Bug 1343754 Opened 7 years ago Closed 7 years ago

gfx-labeling Label runnables in gfx rendering.

Categories

(Core :: Graphics, defect, P1)

Product:
Core
Component:
Graphics
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: vliu, Assigned: kechen)

References

Details

(Whiteboard: [gfx-noted][QDL][TDC-MVP][GFX])

Attachments

(2 files)

Bug 1343754 - Add a function for replacing event target for actor and label PTexture;
59 bytes, text/x-review-board-request
bevis
: review+
nical
: review+
billm
: review+
Details
Bug 1343754 - Label PLayerTransaction;
59 bytes, text/x-review-board-request
bevis
: review+
kats
: review+
Details 
See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story. Currently runnable categories in gfx rendering include sending ipc message between
  content and parent side [1].   

  ./gfx/layers/ipc/PImageBridge.ipdl
  ./gfx/layers/ipc/PLayerTransaction.ipdl
  ./gfx/layers/ipc/PTexture.ipdl
  ./gfx/layers/ipc/PWebRenderBridge.ipdl
  ./gfx/layers/ipc/PCompositorBridge.ipdl

  [1]; https://hg.mozilla.org/mozilla-central/annotate/e150eaff1f83e4e4a97d1e30c57d233859efe9cb/ipc/chromium/src/base/message_loop.cc#l286
Blocks: gfx-labeling
Whiteboard: [gfx-noted]
Assignee: nobody → kechen
There are five ipc protocols under layers folder.
However, only three of them need to be labeled, and I will only do PLayerTransaction and PTexture in this bug.

1. The actors of PImageBridge are in main thread of compositor process and off-main of content process which do not need to be labled.
2. PWebRenderBridge is not completed yet, so I will leave this work in the future.
3. Messages in PCompositorBridge belong to different TabGroups, the messages should be labeled by messages, we will solve this problem in Bug 1333968.
Hello Bevis, could you help me to review the patches for labeling part ? I will find someone to review graphics-related part later.
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review124752

The way to set event target for an actor child is correct but there is still something we can refine.
Please see my comments below.

Thanks!

::: gfx/layers/ipc/CompositorBridgeChild.cpp:36
(Diff revision 2)
>  #include "nsIObserver.h"                // for nsIObserver
>  #include "nsISupportsImpl.h"            // for MOZ_COUNT_CTOR, etc
>  #include "nsTArray.h"                   // for nsTArray, nsTArray_Impl
>  #include "nsXULAppAPI.h"                // for XRE_GetIOMessageLoop, etc
>  #include "FrameLayerBuilder.h"
> +#include "mozilla/Dispatcher.h"

nit:
This include directive is not necessary because TabGroup.h includes Dispatcher.h

::: gfx/layers/ipc/CompositorBridgeChild.cpp:56
(Diff revision 2)
>  
>  using mozilla::layers::LayerTransactionChild;
>  using mozilla::dom::TabChildBase;
>  using mozilla::Unused;
>  using mozilla::gfx::GPUProcessManager;
> +using mozilla::dom::TabGroup;

You won't need these 2 using declaration because you won't explicitly use these 2 types as explained in the following comments.

::: gfx/layers/ipc/CompositorBridgeChild.cpp:891
(Diff revision 2)
> -                                          const uint64_t& aSerial)
> +                                          const uint64_t& aSerial,
> +                                          const uint64_t& aLayerId)
>  {
> -  return TextureClient::CreateIPDLActor();
> +  PTextureChild* textureChild = TextureClient::CreateIPDLActor();
> +
> +  RefPtr<Dispatcher> dispatcher;

You can remove this line as explained in next comment.

::: gfx/layers/ipc/CompositorBridgeChild.cpp:895
(Diff revision 2)
> +
> +  RefPtr<Dispatcher> dispatcher;
> +  TabChild* tabChild = TabChild::GetFrom(aLayerId);
> +
> +  // Do the DOM labeling.
> +  if (tabChild && aLayerId != 0) {

It's infallible to get a TabGroup from a TabChild and to get an EventTarget from a TabGroup, so you can simplify these labeling as followed:
```cpp
if (tabChild && aLayerId != 0) {
  SetEventTargetForActor(
    textureChild, 
    tabChild->TabGroup()->EventTargetFor(TaskCategory::Other));
  MOZ_ASSERT(textureChild->GetActorEventTarget());
}
```

::: gfx/layers/ipc/PCompositorBridge.ipdl:241
(Diff revision 2)
>    /**
>     * Sent when the child has finished CaptureAllPlugins.
>     */
>    async AllPluginsCaptured();
>  
> -  async PTexture(SurfaceDescriptor aSharedData, LayersBackend aBackend, TextureFlags aTextureFlags, uint64_t id, uint64_t aSerial);
> +  async PTexture(SurfaceDescriptor aSharedData, LayersBackend aBackend, TextureFlags aTextureFlags, uint64_t id, uint64_t aSerial, uint64_t aLayerId);

IMO, the aLayerId is not meaningful to the IPC protocol but is only used for setting EventTarget.

The Alternative way is to 
1. override PCompositorBridgeChild::SendPTextureConstructor in CompositorBridgeChild.
2. Call TextureClient::CreateIPDLActor() in this override function.

Then, you don't have to change anything in parent side.
Attachment #8849389 - Flags: review?(btseng)
Comment on attachment 8849390 [details]
Bug 1343754 - Label PLayerTransaction;

https://reviewboard.mozilla.org/r/122168/#review124768

f=me after the following comments are addressed.

Thanks!

::: gfx/layers/ipc/CompositorBridgeChild.cpp:338
(Diff revision 2)
>  CompositorBridgeChild::AllocPLayerTransactionChild(const nsTArray<LayersBackend>& aBackendHints,
>                                                     const uint64_t& aId,
>                                                     TextureFactoryIdentifier*,
>                                                     bool*)
>  {
> +  RefPtr<Dispatcher> dispatcher;

Remove this line.

::: gfx/layers/ipc/CompositorBridgeChild.cpp:346
(Diff revision 2)
>    c->AddIPDLReference();
> +
> +  TabChild* tabChild = TabChild::GetFrom(c->GetId());
> +
> +  // Do the DOM Labeling.
> +  if (tabChild) {

Same as PTexture, we could simplify as follow:
```cpp
if (tabChild) {
  SetEventTargetForActor(
    c, tabChild->TabGroup()->EventTargetFor(TaskCategory::Other));  
}
```
Attachment #8849390 - Flags: review?(btseng)
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review124796

f=me, after the nit is addressed.
Thanks!

::: gfx/layers/ipc/CompositorBridgeChild.cpp:1043
(Diff revision 3)
> +  // Do the DOM labeling.
> +  if (tabChild && aLayerId != 0) {
> +    nsCOMPtr<nsIEventTarget> target =
> +      tabChild->TabGroup()->EventTargetFor(TaskCategory::Other);
> +    SetEventTargetForActor(textureChild, target);
> +  }

nit:
You can add this right after SetEventTargetForActor() to ensure that the EventTarget is set properly.
MOZ_ASSERT(textureChild->GetActorEventTarget());
Attachment #8849389 - Flags: review?(btseng)
Comment on attachment 8849390 [details]
Bug 1343754 - Label PLayerTransaction;

https://reviewboard.mozilla.org/r/122168/#review124802

f=me, thanks!

::: gfx/layers/ipc/CompositorBridgeChild.cpp:344
(Diff revision 3)
> +
> +  // Do the DOM Labeling.
> +  if (tabChild) {
> +    nsCOMPtr<nsIEventTarget> target =
> +      tabChild->TabGroup()->EventTargetFor(TaskCategory::Other);
> +    SetEventTargetForActor(c, target);

nit:
MOZ_ASSERT(c->GetActorEventTarget());
Attachment #8849390 - Flags: review?(btseng)
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review125748
Attachment #8849389 - Flags: review?(btseng) → review+
Comment on attachment 8849390 [details]
Bug 1343754 - Label PLayerTransaction;

https://reviewboard.mozilla.org/r/122168/#review125750
Attachment #8849390 - Flags: review?(btseng) → review+
Comment on attachment 8849390 [details]
Bug 1343754 - Label PLayerTransaction;

https://reviewboard.mozilla.org/r/122168/#review125932

::: gfx/layers/ipc/CompositorBridgeChild.cpp:338
(Diff revision 4)
> +  TabChild* tabChild = TabChild::GetFrom(c->GetId());
> +
> +  // Do the DOM Labeling.
> +  if (tabChild) {
> +    nsCOMPtr<nsIEventTarget> target =

You can compress this a little to:

// Do the DOM labeling.
if (TabChild* tabChild = TabChild::GetFrom(aId)) {
  ...
Attachment #8849390 - Flags: review?(bugmail) → review+
Attachment #8849389 - Flags: review?(bugmail) → review?(nical.bugzilla)
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review125934

::: gfx/layers/client/TextureClient.cpp:894
(Diff revision 4)
>    SurfaceDescriptor desc;
>    if (!ToSurfaceDescriptor(desc)) {
>      return false;
>    }
>  
> +  uint64_t layerId = 0;

This should be "layersId" instead of "layerId" - here and everywhere else in this patch

::: gfx/layers/client/TextureClient.cpp:896
(Diff revision 4)
>      return false;
>    }
>  
> +  uint64_t layerId = 0;
> +  if (XRE_IsContentProcess() && NS_IsMainThread()) {
> +    layerId = static_cast<ShadowLayerForwarder*>(aForwarder)

I don't think this is guaranteed to always be a ShadowLayerForwarder. In particular, with webrender enabled, this might be a WebRenderBridgeChild. I'm not 100% sure though - I think nical would be better to review this.
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review125940

::: gfx/layers/client/TextureClient.cpp:896
(Diff revision 4)
>      return false;
>    }
>  
> +  uint64_t layerId = 0;
> +  if (XRE_IsContentProcess() && NS_IsMainThread()) {
> +    layerId = static_cast<ShadowLayerForwarder*>(aForwarder)

As Kats said, this can be a WebRenderBridgeChild, or even an ImageBridgeChild.
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review126280

R- for now because we can't static_cast into ShadowLayerForwarder in InitIPDLActor. You can use a virtual upcast method like AsLayerForwarder (or add it if isn't there). Note that texture actors can move between tabs because we share the texture pool between all tabs of a given content process, so I assume that you need to be able to change the label of an existing PTexture actor when that happens. When a texture moves to another forwarder, it takes the branch "if (currentFwd != aForwarder) {" in InitIPDLActor so perhaps there is a bit of glue to add in there.
Also as kats said, please rename "layerId" into "layersId" or "layerForwarderId" because layerId is misleading in this context.
Attachment #8849389 - Flags: review?(nical.bugzilla) → review-
Hello Nicolas,
After looking into the code, I found that the texture recycling mechanism is limited within the same forwarder since TextureClientRecycleAllocator is in CompositableClient[1] and currently I cannot find codes that reuse CompositableClient between tabs.
Therefore, we don't have to reset the eventTarget when doing the texture recycle work.

Please correct me if I miss something.

Thank you.

[1] https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/gfx/layers/client/CompositableClient.h#186
Flags: needinfo?(nical.bugzilla)
Unfortunately we have several (too many) ways to recycle textures. The one you are looking at is used mainly for video frames. The one we use for tiles is shared between tabs. See TextureClientPool.h/cpp, it uses TextureForwarder which is the CompositorBridgeChild.
Flags: needinfo?(nical.bugzilla)
Whiteboard: [gfx-noted] → [gfx-noted][QDL][TDC-MVP][GFX]
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review127132

Looks good! With this patch, VR and video related texture actors are not labelled (I suppose this is intentional but it's worth keeping in mind).
Attachment #8849389 - Flags: review?(nical.bugzilla) → review+
Hello Bill,

We got a problem when labeling the ipc PTexture in this bug.
As comment 20 mentioned, we will recycle the actor within the tabs without unregistering it, which means the owning tab of this protocol might be changed after it is created.

I've tried to change the TabGroup in the code but it ran into the assertion in [1].
Are there any ways to reset the event target without unregistering the actor ?


[1]https://dxr.mozilla.org/mozilla-central/source/ipc/glue/ProtocolUtils.cpp#861
Flags: needinfo?(wmccloskey)
No, there isn't any way to do that. It would be hard to know what to do about messages that are already in the message queue but haven't been dispatched yet. Is there a reason to recycle actors in this way? They're not very expensive to create.
Flags: needinfo?(wmccloskey)
Hello Nicolas,
Based on comment 27, is it possible that an actor is recycled while there are still some messages in task queue that haven't been processed yet ?
Flags: needinfo?(nical.bugzilla)
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #28)
> Hello Nicolas,
> Based on comment 27, is it possible that an actor is recycled while there
> are still some messages in task queue that haven't been processed yet ?

To be clear, the actor here means PTexture.
Hello Sotaro,

Could you help me to take a look at comment 28?
Flags: needinfo?(nical.bugzilla) → needinfo?(sotaro.ikeda.g)
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #28)
> Hello Nicolas,
> Based on comment 27, is it possible that an actor is recycled while there
> are still some messages in task queue that haven't been processed yet ?

:kechen, how did you reproduce the problem? In which platform?
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(kechen)
Flags: needinfo?(kechen)
Priority: -- → P1
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #30)
> Hello Sotaro,
> 
> Could you help me to take a look at comment 28?

With the patch, a crash happened around TextureClientPool and TiledContentClient.
Hello Sotaro,

The whole story is that when doing the ipc message labeling, we bind the actors with a TabGroup so the program will know the event target of incoming messages.
But in the PTexture case, the TabGroup of an actor can be changed while the it is recycled; however, gecko doesn't allow us to do such change, because it would be very hard to deal with the messages that have been in message queue.

My question is that, does the "PTexture actor is recycled while there are still some messages for this actor in message queue" thing will actually happened ?
If we can guarantee that the messages which belong to PTexture will be dispatched before the PTexture actor is recycled, maybe we could loose the restriction and change the TabGroup directly.
Flags: needinfo?(sotaro.ikeda.g)
With the patch crash was happened at "MOZ_RELEASE_ASSERT(aActor->Id() == kNullActorId || aActor->Id() == kFreedActorId)"

>  https://dxr.mozilla.org/mozilla-central/source/ipc/glue/ProtocolUtils.cpp#861

The comment say the following.

>  // We should only call this function on actors that haven't been used for IPC
>  // code yet. Otherwise we'll be posting stuff to the wrong event target before
>  // we're called.


But the function is called for aActor that is recycled.
:kechen, so, is it expected that the current patch cause the crash?
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(kechen)
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #33)
> 
> My question is that, does the "PTexture actor is recycled while there are
> still some messages for this actor in message queue" thing will actually
> happened ?

Layer has several recyclings of TextureClients. TabGroup of an actor could be changed by TextureClients of TiledContentClient and TextureClients of ImageBridge(for video).

Recycling TextureClients of ImageBridge(for video) has guarantee that actor is not in message queue because of way of recycling. But in theory, TextureClients of ImageBridge(for video) could be used by multipe tab groups concurrently.

TextureClients of TiledContentClient has a different recycling mechanism than ImageBridge(for video). It tries to avoid it, but architectural guarantee is not clear enough. If it is broken by a bug, we do not have a clear way to detect it.
(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> :kechen, so, is it expected that the current patch cause the crash?

Yes, the patch should crash because this case is not covered by current mechanism.
Flags: needinfo?(kechen)
Hello Bill,

About the PTexture actor recycling problem we've discussed in Comment 27.
Since we recycle the TextureClients frequently in the program, it might be not a good idea to recreate the actor every time.
Also, we can guarantee that there won't be any messages for PTextureChild in the task queue while we are changing the event target of the actor.

Therefore, when doing the texture recycling, I unregistered the actor first and reassigned the new event target for it [1].
Is it a proper way to solve this problem?

TL; DR
After the investigation, the only message that PTextureChild can receive on child side's main thread is __delete__[2].
And the __delete__ message can only be sent by parent process after the child side sends Destroy to parent.

In other words, the only time that we might get the PTexture's message in content side's task queue is after PTextureChild send the Destroy message. And we won't recycle a Texture which has been destroyed [3].

Therefore, it is guaranteed that there won't be any messages in the task queue while we are changing the event target of the actor.

[1] https://reviewboard.mozilla.org/r/122166/diff/6#index_header
[2] https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/gfx/layers/ipc/PTexture.ipdl#29
[3] https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/gfx/layers/client/TextureClient.cpp#866
Flags: needinfo?(wmccloskey)
Depends on: 1350828
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review133054

I don't think this will work.

::: gfx/layers/client/TextureClient.cpp:871
(Diff revision 6)
>  
>    if (mActor && !mActor->mDestroyed) {
>      CompositableForwarder* currentFwd = mActor->mCompositableForwarder;
>      TextureForwarder* currentTexFwd = mActor->mTextureForwarder;
>      if (currentFwd != aForwarder) {
> +      mActor->Unregister(mActor->Id());

Once you unregister an actor, it is removed from the actor map. I don't see where it would get added back to the map. That seems very broken.
Attachment #8849389 - Flags: review?(wmccloskey) → review-
I think we'll need to add a new SetEventTargetForActor method that specifically works even when the actor is already registered. Then we can call that method before SendDestroy.

However, it seems like we should just be able to eliminate this kind of actor the same way we've done for Layer and Compositable. I don't understand what purpose it serves. Would that be possible Matt?
Flags: needinfo?(wmccloskey) → needinfo?(matt.woodrow)
Note that we don't have time to deal with the fallout from any unexpected changes so I would suggest we stick wit the actor recycling unless we're 100% certain we don't need it.
Flags: needinfo?(nical.bugzilla)
(In reply to Bill McCloskey (:billm) from comment #42)
> I think we'll need to add a new SetEventTargetForActor method that
> specifically works even when the actor is already registered. Then we can
> call that method before SendDestroy.
> 
> However, it seems like we should just be able to eliminate this kind of
> actor the same way we've done for Layer and Compositable. I don't understand
> what purpose it serves. Would that be possible Matt?

David was looking into this, and iirc the conclusion was that it was possible, but not entirely trivial since there's a lot of complex code dealing with textures.

There wasn't an obvious huge advantage to doing this work (and non-zero risk), so it's been put aside for now.
Flags: needinfo?(matt.woodrow)
> the conclusion was that it was possible, but not entirely trivial since there's a lot of complex code dealing with textures.

I agree.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8849389 [details]
Bug 1343754 - Add a function for replacing event target for actor and label PTexture;

https://reviewboard.mozilla.org/r/122166/#review137498

Thanks. Sorry this took so long.

::: gfx/layers/client/TextureClient.cpp:12
(Diff revision 7)
>  #include <stdint.h>                     // for uint8_t, uint32_t, etc
>  #include "Layers.h"                     // for Layer, etc
>  #include "gfx2DGlue.h"
>  #include "gfxPlatform.h"                // for gfxPlatform
>  #include "mozilla/Atomics.h"
> +#include "mozilla/dom/TabGroup.h"

I don't think you should need this.

::: gfx/layers/ipc/CompositorBridgeChild.cpp:37
(Diff revision 7)
>  #include "nsISupportsImpl.h"            // for MOZ_COUNT_CTOR, etc
>  #include "nsTArray.h"                   // for nsTArray, nsTArray_Impl
>  #include "nsXULAppAPI.h"                // for XRE_GetIOMessageLoop, etc
>  #include "FrameLayerBuilder.h"
>  #include "mozilla/dom/TabChild.h"
> +#include "mozilla/dom/TabGroup.h"

Same here.

::: ipc/glue/ProtocolUtils.cpp:906
(Diff revision 7)
> +  // The EventTarget of a ToplevelProtocol shall never be set.
> +  MOZ_RELEASE_ASSERT(aActor != this);
> +
> +  int32_t id = aActor->Id();
> +  // The ID of the actor should have existed.
> +  MOZ_RELEASE_ASSERT(id);

Please instead assert it's != kNullActorId and != kFreedActorId.
Attachment #8849389 - Flags: review?(wmccloskey) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25f2e9e2068d
Add a function for replacing event target for actor and label PTexture; r=bevistseng,billm,nical
https://hg.mozilla.org/integration/autoland/rev/a5037670cb10
Label PLayerTransaction; r=bevistseng,kats
Keywords: checkin-needed
Already updated the patches, looks like I failed to upload them to the reviewboard.
I will push another try and mark checkin-needed again if everything is okay.
Flags: needinfo?(kechen)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0630447dd5b6
Add a function for replacing event target for actor and label PTexture; r=bevistseng,billm,nical
https://hg.mozilla.org/integration/autoland/rev/e92e9340219c
Label PLayerTransaction; r=bevistseng,kats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0630447dd5b6
https://hg.mozilla.org/mozilla-central/rev/e92e9340219c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: