Closed Bug 889614 Opened 7 years ago Closed 6 years ago

Plugin content reloads when reparented

Categories

(Core :: Plug-ins, defect, P2)

22 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
firefox25 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: bugzilla, Assigned: johns)

References

Details

(Keywords: regression, reproducible)

Attachments

(5 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Load page containing flash inside of an element that is initially visible, but able to be toggled hidden/visible.

Toggle visibility so that the flash content is hidden.

Toggle visibility again so that the flash content is visible once more.


Actual results:

The flash content re-loaded back to its initial state.


Expected results:

The flash content should be in exactly the same state as if it was visible the entire time.
This looks very much like a regression of this issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=90268

It was in fact fixed back in FF13, but as of FF22 (maybe earlier, just noticed it happening again) it has returned.
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/1d6fe70c79c5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320044721
Bad:
http://hg.mozilla.org/mozilla-central/rev/a73a2b5c423b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130321 Firefox/22.0 ID:20130321045242
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d6fe70c79c5&tochange=a73a2b5c423b


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0f2f7f9a1dc7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320133742
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/992ba60ba625
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130320 Firefox/22.0 ID:20130320144440
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0f2f7f9a1dc7&tochange=992ba60ba625

Suspected: Bug 784131 - Flash players continues to play after closing the video on that site
Blocks: 784131
Status: UNCONFIRMED → NEW
Component: Untriaged → Plug-ins
Ever confirmed: true
Product: Firefox → Core
Sorry, ignore comment#2
No longer blocks: 784131
Status: NEW → UNCONFIRMED
Ever confirmed: false
:drcheap,
Please provide a testcase.
Flags: needinfo?(bugzilla)
Here is a test page containing a reference to the adobe flash sample movie with three javascript links to toggle respectively css display, dom parent and css visibility. You can notice that toggling visibility is ok (leaves the animation in its current state), whereas toggling css display or reparenting the movie unexpectedly reinitializes it.
Flags: needinfo?(bugzilla)
Attachment #770710 - Attachment mime type: text/plain → text/html
(In reply to caribou from comment #5)
> Created attachment 770710 [details]
> A test page which allows to toggle css display, dom parent and css
> visibility of a flash object. In three cases, flash movie should not be
> reloaded.

The relevant whatwg sections should be [1] & [2].

The display:none behavior is correct: it leads to the plugin element not "being rendered" and hence being stopped.
AFAICT not stopping it for toggling visibility is also fine.

That leaves the reparenting, which i'm not sure about. 
[1] says that a task is to be queued which checks the element state after it was inserted/removed from the document. Once that task runs, the reparenting should be already done, so i'd think the check should find the element in the "same" state and probably leave it untouched? It doesn't seem to be explicit about that though.

johns, i think that is something you have an understanding of?

[1] http://www.whatwg.org/specs/web-apps/current-work/#the-object-element 
[2] http://www.whatwg.org/specs/web-apps/current-work/#being-rendered
Flags: needinfo?(jschoenick)
Priority: P2 → --
Version: 22 Branch → Trunk
                       Nightly25.0a1  Aurora24.0a2   Beta23.0b2
toggle css display     restart        restart        restart
toggle dom parent      restart        restart        restart
toggle css visibility  not restart    not restart    not restart


                       Firefox22    Firefox17.0.7esr
toggle css display     restart      not restart
toggle dom parent      restart      restart(but not appear in 2nd div)
toggle css visibility  not restart  not restart



                       Chrome27.0.1453.116  Opera12.15  
toggle css display     restart              restart     
toggle dom parent      restart              restart     
toggle css visibility  not restart          not restart 


                       IE9          IE11       
toggle css display     not restart  not restart
toggle dom parent      not restart  not restart
toggle css visibility  not restart  not restart



So, Only IE(trident) behaves different now.
For details, this problem vas solved back in FF13, see https://bugzilla.mozilla.org/show_bug.cgi?id=90268

I add just the fact that in FF21 it did not restart in any case. 

                       Firefox22    Firefox21
toggle css display     restart      not restart
toggle dom parent      restart      not restart
toggle css visibility  not restart  not restart

IMHO, reparenting restart is really annoying. Indeed, css display restart is not a big issue, since "hiding" the object to the user can still be done without restart by playing with css visibility. On the other way, there is no workaround for reparenting, that's why I find reparenting restart very annoying.
A change to CSS visibility only hides it, however, but it still "takes up space" in the rendered page -- this is the undesirable part.

I have created a dummy version of my application as a more complicated test case that you can mess with.  Please see:  http://wwwtest.industrialinfo.com/bug889614

1. Once the application is loaded, it defaults to the "Topline Installed Base" and "North America" tabs.

2. If you choose some other various tab/menu options such as "Manufacturer Market Share" (I choose this as it's a pie chart, much more obvious that something changes given static dummy data) then click on one of the resulting graph data points, then you will be taken to a results index listing some details.

3. If you then click the "Return To Graph" button, you will then see the SWF is restarted and you are back to the default tabs/graph, whereas in FF13-FF21 the non-restart resulted in returning to the correct location.
As Georg noted, this is proper according to the spec -- display:none plugins should be stopped, visibility:hidden should not be.

However, reparenting should also not respawn the plugin, and we have specific code to ensure that it doesn't, which sounds like we regressed :(

(this is the case for all plugins, not just flash)
Assignee: nobody → jschoenick
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(jschoenick) → in-testsuite?
Keywords: flashplayer
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Embedded flash content reloads when toggled hidden/visible → Plugin content reloads when reparented
Actually this doesn't seem to have worked any time recently for this test, and there is an in-tree test[1] that works. I suspect flash is re-entering into the event loop on NPP_Destroy and causing our pending event to fire :(

[1] http://dxr.mozilla.org/mozilla-central/source/dom/plugins/test/mochitest/test_instance_re-parent.html
Priority: -- → P2
Agreed, it does make sense that display:none would stop the plugin as per the spec.

Fortunately I can work around that in my application by passing state information along with the call to my javascript that hides it.  Then when redisplaying it, I can make a new javascript call to retrieve the state information.  A bit ugly & slower, but effective nonetheless.

As for the re-parenting issue...well good luck with that!
Did something specific cause this to regress between ff21 and ff22? bug 90268, comment 276 seems to imply that at least. Would it be useful to determine what?
(In reply to Timothy Nikkel (:tn) from comment #13)
> Did something specific cause this to regress between ff21 and ff22? bug
> 90268, comment 276 seems to imply that at least. Would it be useful to
> determine what?

bug 784131 landed, which re-adds killing display:none plugins. If the test case there is using display:none, that change is intentional. Otherwise, bug 784131 might've been what broke this.
So this was caused by bug 784131 - we check if we are still missing a frame after returning to the event loop, similar to the out-of-document check, but layout is asynchronous...
Blocks: 784131
Keywords: regression
In 784131 we return to the event loop and see if we have lost our frame, but we should also be flushing layout at this point. We already track the expected CheckPluginStopEvent, so use that variable as a re-entrance guard (if the tag changes state it will be cleared from under us).
Attachment #779496 - Flags: review?(joshmoz)
These tests should've caught this, but weren't thorough enough. This also merges the test_instance_re-parent_windowed test into the non-windowed one.
Attachment #779497 - Flags: review?(joshmoz)
I broke display:none killing with a bad version of this patch, then discovered the tests still passed, then discovered that part of this test was broken too...
Attachment #779499 - Flags: review?(joshmoz)
Version: Trunk → 22 Branch
Comment on attachment 779496 [details] [diff] [review]
Fix regression in plugin reparenting

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +165,5 @@
>    nsObjectLoadingContent *objLC =
>      static_cast<nsObjectLoadingContent *>(mContent.get());
>  
>    // If objLC is no longer tracking this event, we've been canceled or
> +  // superceded. We clear this before we finish - either by calling

Nitpicky, but it's "superseded"
Attachment #779496 - Flags: review?(joshmoz) → review+
Attachment #779497 - Flags: review?(joshmoz) → review+
Attachment #779499 - Flags: review?(joshmoz) → review+
fixed nit
Attachment #779496 - Attachment is obsolete: true
Attachment #781207 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/78209b0be503
https://hg.mozilla.org/mozilla-central/rev/ef4b58b54480
https://hg.mozilla.org/mozilla-central/rev/d600a95c1279
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Oversight - setting mPendingEvent to null should not happen if another event took our place.
Attachment #781881 - Flags: review?(joshmoz)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #781881 - Flags: review?(joshmoz) → review+
https://hg.mozilla.org/mozilla-central/rev/3f5f81744326
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
I've just tested it on FF25 and it works. I don't really know the bug status meanings : Does the following status mean the bugfix can land for FF23 (seems short) and 24 too ?
status-firefox23:	 affected
status-firefox24:	 affected
Regards
23.0 RC is released so the patch won't land in 23.0.
Comment on attachment 781207 [details] [diff] [review]
Fix regression in plugin reparenting. r=josh

[Approval Request Comment]
This isn't terribly important, but flip-flopping between behaviors is likely to be annoying to web authors, so fixing it sooner is preferable to avoid site breakage.

Bug caused by (feature/regressing bug #): Bug 784131 (FF22)

User impact if declined: Regression in plugin behavior (flash respawns when moved between parent nodes). Switching back and forth between behaviors is likely to be annoying to web authors and break some sites.

Testing completed (on m-c, etc.): On m-c

Risk to taking this patch (and alternatives if risky): Low, fixes specific layout race condition along with tests.

String or IDL/UUID changes made by this patch: None
Attachment #781207 - Flags: approval-mozilla-aurora?
Comment on attachment 779499 [details] [diff] [review]
Fix test_object display:none checking issues found while testing

(comment 29)
Attachment #779499 - Flags: approval-mozilla-aurora?
Comment on attachment 779497 [details] [diff] [review]
Fix and cleanup plugin re-parenting tests to catch this in the future

(comment 29)
Attachment #779497 - Flags: approval-mozilla-aurora?
Comment on attachment 781881 [details] [diff] [review]
Followup, don't erroneously cancel other events

(comment 29)
Attachment #781881 - Flags: approval-mozilla-aurora?
Attachment #779497 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 779499 [details] [diff] [review]
Fix test_object display:none checking issues found while testing

given its low risk and a Fx22 regression, has tests and would avoid annoyance to web authors, this is ok to land.
Attachment #779499 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 781207 [details] [diff] [review]
Fix regression in plugin reparenting. r=josh

Please add qawanted in case there are any other testcases that can be verified given this did not have enough baketime on aurora.
Attachment #781207 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #781881 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 781207 [details] [diff] [review]
Fix regression in plugin reparenting. r=josh

I think this was intended to be a+
Attachment #781207 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora?
Attachment #781207 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I see this issue in FF24 ESR. Can you please tell if it has been fixed there?
It is fixed in FF24, which is what ESR 24 started from.
Can you file a new bug with some more details?
Back in FF44 and FF45
Back (or still) in FF44 and FF45

It happens when you change the 'display' property of a a plugin parent element to 'none'; then the NPP_Destroy api is called and the plugin destroyed. If you set back the parent element back to visible a new plugin is created.

This doesn't happen when the 'visibility' property is set to 'hidden'; the current plugin is just hidden.
You need to log in before you can comment on or make changes to this bug.