120875 - Flash contents responds to mouse even if another tab opens

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Koike Kazuhiko]

Flash contents responds to mouse even if another tab opens.

(1) Load <http://www.lordoftherings.net/> in a tab.
(2) Switch to another tab and move mouse

Actual result: Flash contents in <http://www.lordoftherings.net/>
responds to mouse and plays the sound.

Expected result: Flash contents 'sleeps' if the tab is hidden.

This bug is reproduced with Mac builds only.

Comment 1

[jag (Peter Annema)]

nsbeta1, -> 1.0

Comment 2

[Peter Trudelle]

nsbeta1+ per Nav triage team, Nav3, ->1.0

Comment 3

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 4

[Peter Trudelle]

converting nav3->adt3

Comment 5

[Peter Trudelle]

->plugins, plugins should be turned off when the content document is hidden.

Comment 6

[rubydoo123]

reassigning to peterl

Comment 7

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 8

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 9

[Peter Lubczynski]

adt2, I have a fix for this in my trunk and this fixes another bug with print
preview. Bumping up per meeting.

Comment 10

[Peter Lubczynski]

Attachment: patch v.1

Here's a patch that fixes the tabbing problems with plugins (this bug), plugins
showing in print preview (bug 133992), and problems with CSS visibility
property on Mac (bug 137231).

Patch Mac features:
 * Call |FixUpPluginWindow| in |DidReflow|
 * Cleaned up |DidReflow|
 * |FixUpPluginWindow| will now walk up the widgets looking for a hidden one
 * |SetWindow| will be called with a NULL or current port if visibility changes
 

Please review.

Comment 11

[av (gone)]

Comment on attachment 81648 [details] [diff] [review]
patch v.1

This is cool patch, Peter. Although I cannot test it I think it is looking good
codewise. The only comment is: should not we check |window| for null here
before dereferencing it:

+  if (isVisible != mWidgetVisible) {
+     mWidgetVisible = isVisible;
+     nsPluginWindow *window;
+     GetWindow(window);
+     window->window = mWidgetVisible ? GetPluginPort() : nsnull;
+     mInstance->SetWindow(window);

Other than that -- r=av

Comment 12

[Peter Lubczynski]

Attachment: patch v.2

Updated patch:

* Addressed Andrei's comment on checking 'window'
* Now I check the view's visiblity instead of calling |IsHidden| because it is
more accurate

Comment 13

[av (gone)]

Comment on attachment 81739 [details] [diff] [review]
patch v.2

r=av

Comment 14

[Patrick C. Beard]

Comment on attachment 81739 [details] [diff] [review]
patch v.2


+  nsCOMPtr<nsIPluginInstance> pi; 
+  if (!mInstanceOwner || 
+      NS_FAILED(mInstanceOwner->GetWindow(window)) ||
+      NS_FAILED(mInstanceOwner->GetInstance(*getter_AddRefs(pi))) ||
+      !pi)
     return rv;

What's the value of rv in this case? You didn't capture any of the return
values of the calls. In fact, all over the method |nsObjectFrame::DidReflow| we
are returning an rv that is likely to be NS_OK.


+  PRBool isVisible = nsViewVisibility_kShow ? PR_TRUE : PR_FALSE;

Guessing you meant to write:


+  PRBool isVisible = (vis == nsViewVisibility_kShow) ? PR_TRUE : PR_FALSE;

Also, walking our our widget hierarchy each and every time we get redrawn seems
expensive. Isn't there a way we could HEAR about getting hidden, perhaps
through some kind of listener interface, or an event?

Comment 15

[Peter Lubczynski]

Attachment: patch v.3

New patch, made some changes:

* "rv" is set by the container frame's DidReflow, but I propigate the other
return codes now as well.

* There is no notification sent when a widget is hidden or shown, so walking up
is probably better than the widget notifying all its children. I made use of
the existing walk up the parent widgets to also capture visiblity state so we
gather up all the info in just one walk.

Patrick, what do you think?

Comment 16

[Peter Lubczynski]

This fix will probably also stop the "bleed-through" problem I've heard 
about in Chimera tabs although I don't have a build to verify.

Comment 17

[Peter Lubczynski]

Attachment: patch v.4

New patch applies the same fix to full-page plugins.

Comment 18

[Patrick C. Beard]

Comment on attachment 81991 [details] [diff] [review]
patch v.4

sr=beard

Comment 19

[Peter Lubczynski]

Patch in trunk, marking FIXED and nominating adt1.0.0.

Comment 20

[Peter Lubczynski]

*** Bug 133992 has been marked as a duplicate of this bug. ***

Comment 21

[Peter Lubczynski]

*** Bug 137230 has been marked as a duplicate of this bug. ***

Comment 22

[Jaime Rodriguez, Jr.]

Let's get this one in for RTM. adt1.0.0- [adt2 RTM]

Comment 23

[scottputterman]

Adding adt1.0.0+ for 1.0 branch checkin approval.  Please get drivers approval
before checking in.

Comment 24

[Terri Preston]

This is fixed on Mac OS 9 trunk build 2002051408

Comment 25

[Peter Lubczynski]

This isn't quite ready for the branch yet. Some plugins don't like the
SetWindow(NULL) call. See bug 142582 for the fix. Removing graffiti until this
can be sorted out.

Comment 26

[hugo van woerkom]

I do NOT see the described behavior in any of these:
1.0 RC1        Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0rc1) Gecko/20020417
1.0 RC2        Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0rc2) Gecko/20020510
1.0 RC2        Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0rc2) Gecko/20020513
Debian/1.0rc2-1
Mozilla 1.0.0+ Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0+) Gecko/20020511
Debian

Comment 27

[Syd Logan]

Could someone attach a patch specific for mozilla 1.0.0 branch? This patch did
not apply easily there.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Sounds like this depends on bug 142582, and merging the patch may be tricky.

Comment 29

[rubydoo123]

yes, we need to do the merge first, so ignore this one for the branch at the moment

Comment 30

[Peter Lubczynski]

...working on a better fix now, this may have to be backed out though.

Comment 31

[Peter Lubczynski]

reopening

Comment 32

[Peter Lubczynski]

*** Bug 142582 has been marked as a duplicate of this bug. ***

Comment 33

[Brian Nesse (gone)]

*** Bug 141416 has been marked as a duplicate of this bug. ***

Comment 34

[Peter Lubczynski]

Attachment: patch v.5 (for trunk)

This patch fixes the regression in bug 142582 on the trunk:

This is a much safer way of fixing the problem:
1. When we are "hidden", simply clear our clip
2. Send bogus 20000,20000 location for mouse in null events when "hidden"
3. No more risky extra SetWindow calls. They were not needed to begin with.

Comment 35

[Peter Lubczynski]

*** Bug 144926 has been marked as a duplicate of this bug. ***

Comment 36

[Brian Nesse (gone)]

Comment on attachment 83898 [details] [diff] [review]
patch v.5 (for trunk)

Patch looks solid and appears to do the right thing. r=bnesse.

Comment 37

[Patrick C. Beard]

Comment on attachment 83898 [details] [diff] [review]
patch v.5 (for trunk)

Hmmm, I guess if the plugin doesn't call GetMouse() itself this will work, but
this seems like a strange way to implement this.
sr=beard

Comment 38

[Peter Lubczynski]

back to FIXED status, patch in trunk

Comment 39

[rubydoo123]

removed reference in whiteboard about [needs fix from bug 142582], that has
already been added to the patch Peter has for this bug

Comment 40

[Peter Lubczynski]

Attachment: patch v.6 (for branch)

Same patch as before, just merged to the branch.

Comment 41

[Brian Nesse (gone)]

Comment on attachment 85457 [details] [diff] [review]
patch v.6 (for branch)

+  if (!mInstanceOwner ||
+      NS_FAILED(rv = mInstanceOwner->GetWindow(window)) || 
+      NS_FAILED(rv = mInstanceOwner->GetInstance(*getter_AddRefs(pi))) ||
+      !pi ||
+      !window)
This seems like a bit of overkill. I'd probably just insure that pi & window
aren't null and skip the NS_FAILED).

Either way, r=bnesse.

Comment 42

[Patrick C. Beard]

Comment on attachment 85457 [details] [diff] [review]
patch v.6 (for branch)

sr=beard

Comment 43

[Judson Valeski]

"please land this on the 1.0.1 branch. once there remove the "mozilla1.0.1+"
keyword, and add the "fixed1.0.1"

Comment 44

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0.1. pls land this on
the 1.0 branch, then add the keywrod "fixed1.0.1".

Comment 45

[Peter Lubczynski]

fixed1.0.1

Comment 46

[shrirang khanzode]

verified 1.0.1 on os x and os 9.2.