1289859 - Flash Player beta23.0.0.111 does not work in windowed and opaque cases (but 22.0.0.209 works properly)

Status: RESOLVED WORKSFORME

(Core Graveyard :: Plug-ins, defect)

Description

[Alice0775 White]

Build Identifier:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ebd239b4cbfb8eff833560f72b163275ac4026d3
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 ID:20160727004019

The problem does not happens on Firefox48.0 with/without e10s.
The problem happens on 49.0a2 and 50.0a1 with/without e10s.

*Flash Player 22.0.0.209 works without the problem.

Pushlog:


Steps To Reproduce:
1. Install Flash Player beta23.0.0.111
2. Open http://emk.name/test/swftxt.html
3. Try to type something with keyboard

Actual Results:
Flash Player beta23.0.0.111 is not shown if windowed and opaque property is defined

Expected Results:
All ok


And another site is also affected( http://www.adobe.com/software/flash/about/ ).
The version information box is empty.

Comment 1

[Alice0775 White]

Pushlpg:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c605b6253cdb354a69de5d31e8146b3dbdcf93bb&tochange=e35c83cf4a498271a7b4ac24acc373e77215e4fa

Triggered by: e35c83cf4a49	Sotaro Ikeda — Bug 1276403 - Fix nsPluginFrame::IsOpaque() r=mattwoodrow

Comment 2

[Sotaro Ikeda [:sotaro]]

Hmm, even when UseAsyncRendering() is true, there is a case that async rendering is not used.

Comment 3

[Masatoshi Kimura [:emk]]

Do we need to check AsyncDrawingSupport flag? (See bug 1283274.)

Comment 4

[blinky]

It has become a can reproduce the bug 1275680 with hwa enabled after bug 1276403.

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Masatoshi Kimura [:emk] from comment #3)
> Do we need to check AsyncDrawingSupport flag? (See bug 1283274.)

I locally tested AsyncDrawingSupport flag. The flag was always true when Flash Player beta23.0.0.111 is used. So, the flag could not be used for check if AsyncRendering is used.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Alice0775 White from comment #0)
> Steps To Reproduce:
> 1. Install Flash Player beta23.0.0.111
> 2. Open http://emk.name/test/swftxt.html

When the url was shown, async rendering was actually used only on transparent mode.

Comment 7

[Sotaro Ikeda [:sotaro]]

nsPluginFrame needs to be opaque even when UseAsyncRendering() is true, because AsyncRendering might not be used. It seems better to revert bug 1276403 and add workaround to LayerManagerComposite.

bug 1276403 happened because layer of nsPluginFrame is created as opaque but its content could be non-opaque. If it happens, we need not to skip cleaning the region before compositing, because we skip cleaning the opaque region to improve performance.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update PostProcessLayers() as to handle plugin quirk

Comment 9

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
should be uplifted with bug 1283274

Comment 10

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e50e4560c47c

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8775565 [details] [diff] [review]
patch - Update PostProcessLayers() as to handle plugin quirk

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

Why do we need to propagate the clear region up to the root before subtracting it from the opaque? Why can't we just do that immediately at the current layer level?

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> -----------------------------------------------------------------
> 
> Why do we need to propagate the clear region up to the root before
> subtracting it from the opaque? Why can't we just do that immediately at the
> current layer level?

ContainerLayer of the plugin layer owner is also set as opaque in this case. Therefore we needs to subtract the clear region from the opaque after all of the plugin layers are calculated.

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Therefore we needs to subtract the clear region from the opaque after all of
> the plugin layers are calculated.

Correction:
Therefore we needs to subtract the clear region from the opaque after all of its owner container layers' opaque regions are calculated.

Comment 14

[Sotaro Ikeda [:sotaro]]

Similar comment is in Bug 1276403  Comment 5.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Can we instead just ignore CONTENT_OPAQUE on ContainerLayers and only use it for leaves?

Comment 16

[Sotaro Ikeda [:sotaro]]

Yes, it was done in the first patch in Bug 1276403.

Comment 17

[Matt Woodrow (:mattwoodrow)]

I meant make Layer::IsOpaque return false for all ContainerLayers so that we never add to localOpaque for ContainerLayers, only for leaf layers.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: no-opaque-container

Does this work?

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> Created attachment 8775823 [details] [diff] [review]
> no-opaque-container
> 
> Does this work?

With backout of Bug 1276403, it works. But I am concerning about the performance compared to attachment 8775565 [details] [diff] [review].

When attachment 8775565 [details] [diff] [review], Bug 1275680 Comment 0 was addressed both e10s and non-e10s. But with Bug 1276403 it was addressed only on non-e10s. Somehow, Bug 1275680 Comment 0 did not addressed because of flash side problem. I saw it often during investigating Bug 1276403. I suspect that it could be related to composition performance.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Why would performance be worse?

If the ContainerLayer is actually opaque, then we should still figure that out because we visit all the children and add them to the opaque region.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - no-opaque-container with backout of Bug 1276403

Comment 22

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> Why would performance be worse?
> 
> If the ContainerLayer is actually opaque, then we should still figure that
> out because we visit all the children and add them to the opaque region.

With attachment 8775840 [details] [diff] [review] and e10s is enabled, a layer of chrome process is composited as a background of the plugin's layers. The plugin's layers is created as opaque in content process, therefore, there is no visible region of other layers under plugin's layer.

This does not happen during non-e10s. The plugin's layers is created as opaque and there is no visible region of other layers under plugin's layer.

Comment 23

[Matt Woodrow (:mattwoodrow)]

That makes it sound like we need to treat plugins as transparent in the content process if there is any chance they will be transparent during compositing.

Comment 24

[Sotaro Ikeda [:sotaro]]

It was done in Bug 1276403 and caused this bug, because there seems no good way to know when plugin async rendering is actually used.

Comment 25

[Sotaro Ikeda [:sotaro]]

When async rendering is not used and non-transparent mode, nsPluginFrame needs to be opaque.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Why can't we know if async rendering is being used?

(reply to Sotaro Ikeda [:sotaro] from comment #25)
> When async rendering is not used and non-transparent mode, nsPluginFrame
> needs to be opaque.

Why is this? It doesn't really matter *too* much if we mark a layer as transparent when it isn't really does it?

Comment 27

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> 
> Why is this? It doesn't really matter *too* much if we mark a layer as
> transparent when it isn't really does it?

Sorry, my analysis was wrong. When beta23.0.0.111 is used, links of comment uses async rendering.

When wmode was windowed or opaque mode, the plugin seems not render black color. Instead, it seems to render as transparent color and then background color is used as black color.

Comment 28

[Sotaro Ikeda [:sotaro]]

http://emk.name/test/swftxt.html in comment 0 case, when I changed background color of <body> to black via Inspector, the text was rendered correctly. When I changed the background color to red, texts of windowed or opaque mode changed to red.

Comment 29

[Sotaro Ikeda [:sotaro]]

When nsPluginFrame is opaque, plugin's background color becomes default clear color(#000000), then black text is shown as black.

Comment 30

[Sotaro Ikeda [:sotaro]]

:mattwoodrow, from Comment 28. The problem seems to be caused by beta23.0.0.111. Is it better to be addressed by adobe side? Or should we add workaround for it?

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> When nsPluginFrame is opaque, plugin's background color becomes default
> clear color(#000000),

Because there is no background layer under it. Then DrawTarget::ClearRect() in the following becomes background color of the plugin's layer.
  https://dxr.mozilla.org/mozilla-central/source/widget/CompositorWidget.cpp#51

Comment 32

[Matt Woodrow (:mattwoodrow)]

(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> :mattwoodrow, from Comment 28. The problem seems to be caused by
> beta23.0.0.111. Is it better to be addressed by adobe side? Or should we add
> workaround for it?

Ideally we'd get them to fix it.

(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> (In reply to Sotaro Ikeda [:sotaro] from comment #29)
> > When nsPluginFrame is opaque, plugin's background color becomes default
> > clear color(#000000),
> 
> Because there is no background layer under it. Then DrawTarget::ClearRect()
> in the following becomes background color of the plugin's layer.
>  
> https://dxr.mozilla.org/mozilla-central/source/widget/CompositorWidget.cpp#51

This makes sense I think. If layout thinks the plugin is going to be opaque, then it won't render any content behind the plugin. If the plugin is actually transparent, then we're going to end up showing the ClearRect() colour through when we shouldn't.

Comment 33

[Sotaro Ikeda [:sotaro]]

:masayuki, do you know how to ask adobe to address comment 27?

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I emailed you since it includes their email addresses.

Comment 35

[Sotaro Ikeda [:sotaro]]

Thanks a lot!

Comment 36

[Sotaro Ikeda [:sotaro]]

I heard that this bug is already addressed internally in adobe and a fix will be in next beta release.

Comment 37

[Toshihiro Yamada]

The problem seems to be fixed in Flash Player beta 23.0.0.126.

Comment 38

[Alice0775 White]

Yes, I verified that Flash Player beta 23.0.0.126 fixed the problem.