[e10s] Flash content does not appear

RESOLVED FIXED in Firefox 50

Status

()

Core
Plug-ins
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: blinky, Assigned: sotaro)

Tracking

({regression})

49 Branch
mozilla50
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox50 fixed)

Details

(Whiteboard: STR in comment 0)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
Steps to reproduce:

Open this flash in the same tab or by copy and past the link into the address bar https://dl.dropboxusercontent.com/u/95157096/85f61cf7/3hl97q6u59.swf.

I am using the latest Flash Player beta 22.0.0.168.


Actual results:

It displays a blank page instead of Flash content.

See https://dl.dropboxusercontent.com/u/95157096/85f61cf7/uxqgr9mb9e.mp4
Flags: needinfo?(dvander)
(Reporter)

Comment 1

a year ago
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=02c69f4f896255189ce2f9f4e0d875e383bcfbd7&tochange=a1bd47d76f71162534090485acc57866dcd55eef

Regressed by: bug 1229961
Blocks: 1229961
Keywords: regression
(Reporter)

Updated

a year ago
Blocks: 1217665
(Assignee)

Comment 2

a year ago
Created attachment 8757248 [details] [diff] [review]
patch - InvalidateRect in PluginInstanceParent::SetCurrentImage()
(Assignee)

Comment 3

a year ago
attachment 8757248 [details] [diff] [review] addressed the problem for me.
(Assignee)

Comment 4

a year ago
To reproduce the problem, I used latest "Flash Player beta 22.0.0.168" as in Bug 1275680 Comment 8.
(Assignee)

Comment 5

a year ago
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> To reproduce the problem, I used latest "Flash Player beta 22.0.0.168" as in
> Bug 1275680 Comment 8.

And also direct plugin drawing needs to be enabled.
(Assignee)

Updated

a year ago
Attachment #8757248 - Flags: review?(dvander)
(Assignee)

Updated

a year ago
See Also: → bug 1275680
Comment on attachment 8757248 [details] [diff] [review]
patch - InvalidateRect in PluginInstanceParent::SetCurrentImage()

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

Sotaro, could you explain why this change works? This was designed to not need to invalidate anything in layout: SetCurrentImage bumps the frame ID which should tell the compositor it needs to composite the layer again, and SetCurrentImages should perform an async transaction.
Flags: needinfo?(dvander) → needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 7

a year ago
When ImageContainer is in ImageContainer::ASYNCHRONOUS, ImageClientSingle is created on ImageBridge. ImageHost is also created on ImageBridge. At this point, ImageHost is not connected to ImageLayerComposite. Even when ImageHost receives an image to render, ScheduleComposition() is not called in ReceiveCompositableUpdate()
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositableTransactionParent.cpp#196

To connect ImageHost to ImageLayerComposite, ImageClientBridge::UpdateImage() needs to be called. ImageClientBridge is on CompositorBridge. To call the UpdateImage(), LayerTransaction needs to be triggered. To trigger the layer transaction, the patch called Invalidate.

  https://github.com/sotaroikeda/firefox-diagrams/blob/master/gfx/gfx_ImageBridge_FirefoxOS_2_1.pdf
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 8

a year ago
Created attachment 8758090 [details] [diff] [review]
patch - Call invalidate to trigger ImageClientBridge::UpdateImage()

This patch also worked.
(Assignee)

Comment 9

a year ago
Comment on attachment 8758090 [details] [diff] [review]
patch - Call invalidate to trigger ImageClientBridge::UpdateImage()

:dvander, which patch is better? Or should be address the problem another way?
Attachment #8758090 - Flags: review?(dvander)
(Assignee)

Comment 10

a year ago
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> Comment on attachment 8758090 [details] [diff] [review]
> patch - Call invalidate to trigger ImageClientBridge::UpdateImage()

It might be fragile in a case that layer is recreated compared to attachment 8757248 [details] [diff] [review].

Updated

a year ago
tracking-e10s: ? → -
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> (In reply to Sotaro Ikeda [:sotaro] from comment #9)
> > Comment on attachment 8758090 [details] [diff] [review]
> > patch - Call invalidate to trigger ImageClientBridge::UpdateImage()
> 
> It might be fragile in a case that layer is recreated compared to attachment
> 8757248 [details] [diff] [review].

How does video handle this scenario? Does it delete the ImageContainer? (I'm fine with taking the first patch if we think the new approach is fragile, but I want to understand what's happening first)
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 12

a year ago
During video rendering there is a case that ImageLayer does not exist. VideoFrameContainer::InvalidateWithFlags() handles it. frame->InvalidateFrame() or frame->InvalidateLayer() is called for each video rendering. From it, I just thought the first patch is safer.
  https://dxr.mozilla.org/mozilla-central/source/dom/media/VideoFrameContainer.cpp#169
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> During video rendering there is a case that ImageLayer does not exist.
> VideoFrameContainer::InvalidateWithFlags() handles it.
> frame->InvalidateFrame() or frame->InvalidateLayer() is called for each
> video rendering. From it, I just thought the first patch is safer.
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/media/VideoFrameContainer.
> cpp#169

Okay, thanks for the explanation. The first patch seems reasonable then.
Attachment #8757248 - Flags: review?(dvander) → review+
(Assignee)

Updated

a year ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Updated

a year ago
Attachment #8758090 - Attachment is obsolete: true
Attachment #8758090 - Flags: review?(dvander)
(Assignee)

Comment 14

a year ago
Created attachment 8759526 [details] [diff] [review]
patch - InvalidateRect in PluginInstanceParent::SetCurrentImage()

Fixed warining.
Attachment #8757248 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8759526 - Flags: review+
(Assignee)

Comment 15

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c3126d1f84f

Comment 16

a year ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/189923b75373
InvalidateRect in PluginInstanceParent::SetCurrentImage() r=dvander
(Assignee)

Updated

a year ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/189923b75373
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Updated

a year ago
Blocks: 1275680
See Also: bug 1275680

Updated

9 months ago
Whiteboard: STR in comment 0
You need to log in before you can comment on or make changes to this bug.