529263 - Flash contents in the sidebar are not rendered properly

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Kohei Yoshino [:kohei]]

How to reproduce:

1. Bookmark http://www.adobe.com/ (or an arbitrary Flash site)
2. Open Library, select the bookmark and check "Load this bookmark in the sidebar".
3. Click the bookmark to load in the sidebar

It doesn't work with Firefox 3.6 and trunk while it works with 3.5.x and Windows builds.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20091116 Minefield/3.7a1pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; ja-JP-mac; rv:1.9.2b3) Gecko/20091115 Firefox/3.6b3

Comment 1

[Kohei Yoshino [:kohei]]

I have 10.0 Shockwave Flash 10.0 r32, the latest version.

This build shows a blank rectangle overlay on the main content area as large as the Flash content but the playback goes smoothly:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20091001 Minefield/3.7a1pre

This build doesn't work well:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20091002 Minefield/3.7a1pre

The regression range?

http://hg.mozilla.org/comm-central/pushloghtml?startdate=2009-09-30&enddate=2009-10-02

Comment 2

[Kohei Yoshino [:kohei]]

Not comm-central but mozilla-central.

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-09-30&enddate=2009-10-02

Comment 3

[Kohei Yoshino [:kohei]]

Still reproducible with the latest trunk.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20100104 Minefield/3.7a1pre

This bug affects the J-WAVE Toolbar add-on which has a sidebar web panel with embedded Flash contents:

https://addons.mozilla.org/firefox/addon/48910

Comment 4

[Mike Kaply [:mkaply]]

This seems like a pretty big regression. But no traction for 3.6?

Comment 5

[Kohei Yoshino [:kohei]]

This bug seems be reproducible not only on Mac but also Windows/Linux.

Comment 6

[Mike Kaply [:mkaply]]

I'm upping the severity on this one. This is a major regression. It's breaking quite a few of our extensions.

Comment 7

[Benjamin Smedberg]

The sidebar appears to be a <xul:browser> which is *not* type="content". Are you trying to load a plugin from chrome content? This and bug 534355 are probably the same bug.

The bug here seems to be that the sidebar should be type="content". I don't know what that would break.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> The sidebar appears to be a <xul:browser> which is *not* type="content".

Uh.... it's not?  Why not?  It should be.  It was last I checked, for things other than bookmarks and whatnot!  Was I just confused?  If not, what changed and when?

Comment 9

[Mike Kaply [:mkaply]]

At least in my case, I have my own sidebar.xul in which I put a <browser type="content"> and the flash doesn't render.

The plugin loads (if I right click, I see the flash menu), it just won't load content.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Michael, it sounds like you might be seeing a different bug.  Can you please file it, along with whatever xul file should be run with -chrome to reproduce it?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> The sidebar appears to be a <xul:browser> which is *not* type="content".

How did you observe this? STR from comment 0 result in a sidebar with a <browser type="content"> for me (verified with DOM inspector). We hardcode that here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/web-panels.xul#95

Comment 12

[Kohei Yoshino [:kohei]]

The sidebar panel of the J-WAVE Toolbar (above-mentioned) has <browser> with type="content".

<browser id="jwave-sidebar-frame" src="http://www.j-wave.co.jp/locmag/tlbar_v.htm" type="content" style="width: 100%" flex="1" />

Comment 13

[Mike Kaply [:mkaply]]

I've debugged and this is the same problem we have. And the sidebar IS type=content.

The problem is simply that flash content is not working in the sidebar for some reason.

Here's an example of one of our sidebars:

http://www.thendl.com/NDL-sidebar/sidebar.asp

If you add it as a book mark and open it in the sidebar, it says "Adobe Flash Player required" This is the exact same problem we have with our custom sidebar.

Looking at the Browser DOM node in the bookmark case (web panels)

<browser xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" id="web-panels-browser" persist="cachedurl" type="content" flex="1" context="contentAreaContextMenu" tooltip="aHTMLTooltip" onclick="return window.parent.contentAreaClick(event, true);" cachedurl="http://www.thendl.com/NDL-sidebar/sidebar.asp"/>

It's type="content"

So there was a MAJOR regression displaying flash in the sidebar.

This all works in Firefox 3.5.

Does anyone even have a clue as to why this regressed?

This is a huge problem for us.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK.  So the regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=51710e0ba8a3&tochange=a80414164888 (using the url from comment 13).

Comment 15

[Boris Zbarsky [:bzbarsky]]

This is a regression from bug 508908. In particular, this test:

    if (parentWidget == GetWindow()) {

in nsObjectFrame::CreateWidget tests false for the plug-in in the sidebar.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Well, in this case |rpc| is the toplevel Firefox window prescontext, but GetWindow() returns the widget corresponding to the viewport frame of the sidebar HTML document (or at least stepping into GetWindow has this at the point when we're about to do |return v->GetWidget();| I get:

(gdb) p ((nsIFrame*)v->GetClientData())->PresContext()->mShell->mDocument->mDocumentURI.mRawPtr->mSpec.mData
$26 = 0x20f781d8 "http://www.thendl.com/NDL-sidebar/sidebar.asp"

So it looks to me like the root prescontext is wrong in the sidebar case for some reason...

Comment 17

[Boris Zbarsky [:bzbarsky]]

And in particular, when we're setting up that sidebar.asp document viewer and DocumentViewerImpl::InitInternal calls FindContainerView, it finds the inner view of the web-panels.xul subdoc frame.  And in particular, mParentWidet is not null, so |containerView| ends up non-null as well.  But web-panels.xul is itself loaded in a subframe of browser.xul, so we hit this code in FindContainerView:

2389   // see if the containerView has already been hooked into a foreign view manager hierarchy
2390   // if it has, then we have to hook into the hierarchy too otherwise bad things will happen.
2391   nsIViewManager* containerVM = containerView->GetViewManager();
2392   nsIView* pView = containerView;
2393   do {
2394     pView = pView->GetParent();
2395   } while (pView && pView->GetViewManager() == containerVM);
2396   if (pView)
2397     return containerView;

and hook the sidebar into the chrome viewmanager hierarchy, even though it has type="content".  But the type="content" subframe still has a widget at its root, so we get the observed behavior...

roc, do we really need this "see if ..." codepath at this point?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, I don't think we do.

Comment 19

[Mike Kaply [:mkaply]]

So is the fix to remove all that code?

Any chance we could put that fix on a try server and I could see if it fixes the other flash sidebar problems I'm seeing?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I pushed that change to the try server. I believe builds will show up here:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-a141be2ee10f/
although there's nothing there yet.

Comment 21

[Mike Kaply [:mkaply]]

I've verified that this build all the flash sidebar problems I was seeing.

Comment 22

[Boris Zbarsky [:bzbarsky]]

That sentence is missing a verb....  Is the missing verb the present form of "to have" or the present form of "to fix"?

Comment 23

[Mike Kaply [:mkaply]]

Doh :)

It fixes all of my flash issues in the sidebar.

Comment 24

[Kohei Yoshino [:kohei]]

(In reply to comment #20)
> https://build.mozilla.org/tryserver-builds/[e-mail]-try-a141be2ee10f/

J-WAVE Toolbar's sidebar panel works with this build.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426754 [details] [diff] [review]
fix

Take out the three lines of comment (two text, one blank) right after the code you removed, and r=me

Comment 27

[Mike Kaply [:mkaply]]

The reason I'm requesting blocking is because this was a major regression in Firefox 3.6.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Agreed; please put the patch up for 1.9.2.2 approval when it's branch-safe and after it's baked a cycle on trunk.

Also: I would really, really, really appreciate a test for this to ensure we don't break it again.

Comment 29

[Mike Kaply [:mkaply]]

Is there someone that can check this in?

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/efe73e4926cd

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 426754 [details] [diff] [review]
fix

Fixes a significant regression for extensions in 3.6.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 426754 [details] [diff] [review]
fix

a1922=beltzner, with another plea for tests

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, now I've just got a plea to land it.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/338ee684a7c0

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2 using the testcase in comment 0.

Comment 38

[cpoon2uiuc]

If this was fixed in February, why hasn't it been included in a release if it's such a critical issue??

I ask because I'm using version 3.6.8 and the problem persists.

Comment 39

[Al Billings [:abillings - ex-MoCo]]

It was fixed in 3.6.2 as far as I can tell.

What are your exact steps to reproduce it and what operating system are you using?

Comment 40

[cpoon2uiuc]

Attachment: Problem persisting in version 3.6.8 (Windows 7)

Comment 41

[cpoon2uiuc]

Attachment: Flash failing in sidebar (3.6.8, Windows 7)

Screenshot shows 2 versions of Youtube:  1 in an iframe, 1 in a browser

Comment 42

[cpoon2uiuc]

Clarification:  The <iframe> tags are used in the top frame and the <browser> tags were used in the bottom.  As you can see, there's mush where the flash video should be in the top frame while the video displays properly in the browser frame.

On another note, I don't want to use <browser> tags because (if you look closely), the browser overlaps the edge of the sidebar.  There's a thin grey line on the edge of the iframe but there's none at the edge of the browser.  The overlapping makes it so that the user cannot expand the sidebar to the right.  The option just isn't there!

Comment 43

[Boris Zbarsky [:bzbarsky]]

Can you please file a separate bug on that issue, with clear steps to reproduce?  If you're using a <browser>, then you're not using what this bug means by "sidebar" (which are all content docshells and can't have <browser>s in them).

Comment 44

[Mike Kaply [:mkaply]]

This looks like this bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=424117

Comment 45

[Don Park]

Our sidebar using iframe tag with type='content' displays just fine in version 3.6.8 running on Windows 7 with Flash 10.1.82.76 installed.

Comment 46

[Don Park]

FYI, wmode we used is 'window'. Don't remember if other modes worked or not unfortunately but do remember having issues with other iframe types.

'content' iframe type sandboxes content so it's a hassle to work with but you can use events and DOM elements to bridge over sandbox security.