Last Comment Bug 90268 - move plugins to content - plugins should withstand a reframe of the object frame
: move plugins to content - plugins should withstand a reframe of the object frame
Status: RESOLVED FIXED
: testcase, topembed-
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: P2 normal with 75 votes (vote)
: mozilla13
Assigned To: Josh Aas
:
Mentors:
http://wiki.mozilla.org/Gecko:Moving_...
: 78242 173276 262354 299295 303888 314533 335583 339578 340465 391015 406002 411149 414029 429428 451816 479807 488167 502753 507715 590891 647739 647943 658980 661950 665975 666366 667213 670729 675557 690525 752827 (view as bug list)
Depends on: 723520 723529 724651 767673 865609 874027 1156 135263 285982 519752 660721 665996 674223 674224 675005 682951 683007 683059 683084 697651 707052 713552 719117 723133 723154 723162 723164 723190 723217 723291 723379 723473 723476 723523 723558 724248 724250 724532 724717 724781 724812 725279 725917 726278 726734 727211 728672 736998 737433 753251 776451 784131 788900 825188
Blocks: 300659 633985 706669 88998 136927 158670 grouper 180802 203401 303888 340465 468768 501485 614825 667213 713632 718287
  Show dependency treegraph
 
Reported: 2001-07-10 21:48 PDT by Peter Lubczynski
Modified: 2015-09-04 07:05 PDT (History)
138 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reframe testcase - changing CSS display type (881 bytes, text/html)
2002-03-06 22:00 PST, Peter Lubczynski
no flags Details
Proposed fix for the plugin unload/reload bug (4.55 KB, patch)
2008-06-19 18:11 PDT, Tristan Schmelcher
no flags Details | Diff | Review
Don't asynchronously instantiate in HasNewFrame if already instantiated for that frame. Fixes a race where EnsureInstantiation would instantiate the plugin and then HasNewFrame would re-instantiate it (1.41 KB, patch)
2008-06-23 15:46 PDT, Tristan Schmelcher
no flags Details | Diff | Review
test page to display flash reset problem (1.86 KB, text/xml)
2009-11-06 12:12 PST, Phillip Whelan
no flags Details
Page showing the flash reset problem (2.74 KB, text/html)
2009-11-06 12:28 PST, Phillip Whelan
no flags Details
fix v0.4 (99.75 KB, patch)
2011-06-09 22:41 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.6 (121.47 KB, patch)
2011-06-15 07:54 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.6 + windows build fix (121.92 KB, patch)
2011-06-15 11:43 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details | Diff | Review
fix v0.7 (125.00 KB, patch)
2011-06-16 00:01 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.75 (139.91 KB, patch)
2011-06-25 15:57 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.75 updated to latest m-c (139.85 KB, patch)
2011-06-27 06:47 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.76 (136.75 KB, patch)
2011-07-11 10:43 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.8 (135.93 KB, patch)
2011-07-13 22:43 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.8, updated to current m-c (135.97 KB, patch)
2011-07-22 05:44 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.8, updated to current m-c (134.82 KB, patch)
2011-08-09 09:02 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.8, updated to current m-c (135.12 KB, patch)
2011-08-19 14:09 PDT, Josh Aas
no flags Details | Diff | Review
fix v0.82 (134.45 KB, patch)
2011-08-21 15:09 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.0 (133.91 KB, patch)
2011-08-22 17:24 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.1 (137.15 KB, patch)
2011-08-23 19:17 PDT, Josh Aas
no flags Details | Diff | Review
fix v1.2 (138.69 KB, patch)
2011-08-24 19:53 PDT, Josh Aas
no flags Details | Diff | Review
widget fix v1.3 (7.27 KB, patch)
2011-08-25 14:07 PDT, Josh Aas
roc: review+
karlt: review-
Details | Diff | Review
other fix v1.3 (133.73 KB, patch)
2011-08-25 14:08 PDT, Josh Aas
no flags Details | Diff | Review
widget fix v1.4 (8.32 KB, patch)
2011-08-26 22:43 PDT, Josh Aas
karlt: review+
Details | Diff | Review
other fix v1.4 (128.63 KB, patch)
2011-08-28 10:40 PDT, Josh Aas
no flags Details | Diff | Review
widget fix v1.5 (8.27 KB, patch)
2011-08-28 21:19 PDT, Josh Aas
no flags Details | Diff | Review
other fix v1.5 (136.89 KB, patch)
2011-08-28 21:20 PDT, Josh Aas
roc: review+
Details | Diff | Review
other fix v1.6 (136.88 KB, patch)
2011-08-28 22:30 PDT, Josh Aas
no flags Details | Diff | Review
other fix v1.7 (136.73 KB, patch)
2011-08-30 19:34 PDT, Josh Aas
no flags Details | Diff | Review
other fix v1.8 (142.02 KB, patch)
2011-09-06 06:10 PDT, Josh Aas
no flags Details | Diff | Review
widget fix v1.6 (8.26 KB, patch)
2011-09-23 14:06 PDT, Josh Aas
no flags Details | Diff | Review
other fix v1.9 (141.97 KB, patch)
2011-09-23 14:10 PDT, Josh Aas
no flags Details | Diff | Review
other fix v2.0 (143.52 KB, patch)
2011-09-25 19:19 PDT, Josh Aas
no flags Details | Diff | Review
other fix v2.1 (145.03 KB, patch)
2011-09-27 23:29 PDT, Josh Aas
no flags Details | Diff | Review
widget fix v1.7 (8.25 KB, patch)
2011-10-05 14:12 PDT, Josh Aas
no flags Details | Diff | Review
other fix v2.2 (145.37 KB, patch)
2011-10-05 14:18 PDT, Josh Aas
no flags Details | Diff | Review
other fix v2.3 (144.27 KB, patch)
2011-10-09 21:30 PDT, Josh Aas
no flags Details | Diff | Review
widget fix v1.8 (8.30 KB, patch)
2011-10-19 22:13 PDT, Josh Aas
no flags Details | Diff | Review
other fix v2.4 (143.54 KB, patch)
2011-10-19 22:14 PDT, Josh Aas
no flags Details | Diff | Review
other fix v2.5 (143.60 KB, patch)
2011-10-20 09:35 PDT, Josh Aas
no flags Details | Diff | Review
An other way to get the bug (277 bytes, text/plain)
2011-10-20 21:45 PDT, Mathieu Beausoleil
no flags Details
other fix v2.6 (143.60 KB, patch)
2011-10-25 12:07 PDT, Josh Aas
no flags Details | Diff | Review
widget fix v1.9 (8.36 KB, patch)
2011-10-26 14:25 PDT, Josh Aas
no flags Details | Diff | Review
plugin fix v2.7 (145.56 KB, patch)
2011-10-26 14:28 PDT, Josh Aas
no flags Details | Diff | Review
testcase showing reset on ancestor style change (2.67 KB, text/html)
2011-10-27 20:51 PDT, Karl Tomlinson (ni?:karlt)
no flags Details
plugin fix v2.8 (146.19 KB, patch)
2011-11-03 09:21 PDT, Josh Aas
no flags Details | Diff | Review
plugin fix v2.9 (145.72 KB, patch)
2011-11-03 13:23 PDT, Josh Aas
no flags Details | Diff | Review
widget fix v2.0 (8.36 KB, patch)
2011-11-14 08:25 PST, Josh Aas
no flags Details | Diff | Review
plugin fix 3.0 (147.49 KB, patch)
2011-11-14 08:29 PST, Josh Aas
no flags Details | Diff | Review
plugin fix 3.1 (148.94 KB, patch)
2011-11-15 20:14 PST, Josh Aas
no flags Details | Diff | Review
plugin fix v3.2 (171.26 KB, patch)
2011-11-29 08:12 PST, Josh Aas
no flags Details | Diff | Review
Log of pandora loading w/o this patch. (28.41 KB, text/plain)
2011-11-29 17:59 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details
Log of pandora loading with this patch. (23.39 KB, text/plain)
2011-11-29 18:09 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details
plugin fix v3.3 (162.18 KB, patch)
2011-12-01 11:24 PST, Josh Aas
no flags Details | Diff | Review
plugin fix v3.4 (171.22 KB, patch)
2011-12-01 14:51 PST, Josh Aas
no flags Details | Diff | Review
plugin fix v3.5 (171.24 KB, patch)
2011-12-01 19:33 PST, Josh Aas
no flags Details | Diff | Review
plugin fix v3.6 (176.45 KB, patch)
2011-12-26 15:14 PST, Josh Aas
no flags Details | Diff | Review
plugin fix v3.7 (176.39 KB, patch)
2012-01-09 23:22 PST, Josh Aas
no flags Details | Diff | Review
Mochitest to ensure that we don't deliver the data="" data twice, rev. 1 (5.24 KB, patch)
2012-01-10 10:28 PST, Benjamin Smedberg [:bsmedberg]
jaas: review+
Details | Diff | Review
plugin fix v3.8 (177.74 KB, patch)
2012-01-12 12:54 PST, Josh Aas
no flags Details | Diff | Review
plugin fix v3.9 (178.70 KB, patch)
2012-01-12 21:30 PST, Josh Aas
roc: review+
Details | Diff | Review
plugin fix v4.0 (178.96 KB, patch)
2012-01-19 11:37 PST, Josh Aas
no flags Details | Diff | Review
plugin fix v4.1 (179.25 KB, patch)
2012-01-23 18:22 PST, Josh Aas
benjamin: review+
Details | Diff | Review
plugin fix v4.2 (179.32 KB, patch)
2012-01-27 08:20 PST, Josh Aas
no flags Details | Diff | Review

Description Peter Lubczynski 2001-07-10 21:48:11 PDT
...from bug 89493 (fixing 3d Groove), the object frame has the general problem
of not being able to take a reframe. This is because nothing else has a
reference to the nsPluginInstanceOwner, the plugin gets destroyed when the frame
gets destroyed. This should be fixed so that we can change the object frame
without messing with the plugin instance owner.

See meta bug 88998 about cleaning up plugin code and bug 89077 about
nsPluginInstanceOwner getting it's own file.
Comment 1 Peter Lubczynski 2001-07-10 21:54:55 PDT
rephrasing...
Comment 2 Chris Waterson 2001-08-03 13:55:35 PDT
lemme try to tackle this for 0.9.4.
Comment 3 Hansoo Kim 2001-11-07 13:34:53 PST
In embedding case, page with shockwave flash after font downloading
does not get rendered.
You need to manually hit reload button to render it right.

I believe it's related to http://bugzilla.mozilla.org/show_bug.cgi?id=91250

A couple of interesting points.
1) Only Korean sites with flash does not render at all
   after font downloading.
2) All other languages ( two Chinese and Japanese ) sites
   rendered strange.
   --> Flash overlapping the text in the middle of the page, etc.
3) Saving korean sites in local drive and loading it works fine and rendered
   correctly even after font downloading.
4) For other lang sites, loading from local drive still renderes the pages 
   right.
Comment 4 Teruko Kobayashi 2001-11-07 15:48:25 PST
I nominate this to edt0.9.4.
Comment 5 Judson Valeski 2001-11-15 15:01:35 PST
are we close to a patch here?
Comment 6 Judson Valeski 2001-11-20 12:21:28 PST
minusing. 0.9.4 is not targeted for broad i18n distribution, this can wait.
Comment 7 Peter Lubczynski 2001-11-28 14:41:22 PST
-->over to Andrei who I think is going to work on this....

There is a starter patch by Waterson here:
http://bugzilla.mozilla.org/attachment.cgi?id=41015&action=view
Comment 8 av (gone) 2001-12-11 17:58:58 PST
I don't think I can do it for 0.9.7. Moving.
Comment 9 av (gone) 2002-01-09 17:59:27 PST
waterson, did you make any progress on this bug while it was assigned to you?
Comment 10 av (gone) 2002-01-09 18:01:28 PST
BTW, Peter, fixing this bug could help to avoid the crash in bug 116232 even if 
that bug itself is not fixed.
Comment 11 Chris Waterson 2002-01-09 18:02:43 PST
Other than attachment 41015 [details] [diff] [review] (which is probably nice and bit-rotten by now), I
didn't do anything.
Comment 12 av (gone) 2002-01-14 13:14:32 PST
This is a big effort, I do not think we can do it into 0.9.4.
Comment 13 Peter Lubczynski 2002-02-18 10:42:24 PST
nominating nsbeta1
Comment 14 Jaime Rodriguez, Jr. 2002-02-21 15:07:31 PST
nsbeta1+ per ADT Triage, reassigning to peterl
Comment 15 Peter Lubczynski 2002-03-06 22:00:51 PST
Created attachment 72926 [details]
reframe testcase - changing CSS display type

This testcase shows that changing the CSS display type of the EMBED tag or even
its surrounding tags causes a reframe and the plugin animation to restart.
Comment 16 rubydoo123 2002-03-13 11:30:25 PST
Moving this to 1.1, this is going to require significant rewrite of the plug-in 
layout code and is just too risky to do at this point in time.
Comment 17 saari (gone) 2002-03-25 12:55:30 PST
topembed+ because this is known to be a large issue affecting plugins
Comment 18 Peter Lubczynski 2002-04-03 14:09:20 PST
I think we need to figure out how to reparent the plugin's widget before this
can be fixed. Marking depends on bug 135263.
Comment 19 Dave Barrowman 2002-04-23 13:01:03 PDT
Peter, is there any update on this one?
Comment 20 Peter Lubczynski 2002-04-23 15:43:31 PDT
I think this may require a substantial amount of engineering work in re-writing
plugin layout code. At this point in time, I think it's pretty high risk.
Comment 21 Peter Lubczynski 2002-05-07 15:12:39 PDT
*** Bug 78242 has been marked as a duplicate of this bug. ***
Comment 22 Peter Lubczynski 2002-07-18 18:25:16 PDT
moving plugins to content would increase stability and is scheduled for plugins 2.0
Comment 23 Boris Zbarsky [:bz] 2002-10-08 19:25:06 PDT
*** Bug 173276 has been marked as a duplicate of this bug. ***
Comment 24 rubydoo123 2002-12-05 15:35:35 PST
moving to 1.4 beta, plug-in branch work
Comment 25 Peter Lubczynski 2003-01-22 13:33:11 PST
per topembed+ triage: topembed-, Future
Comment 26 Boris Zbarsky [:bz] 2005-10-30 12:34:30 PST
*** Bug 299295 has been marked as a duplicate of this bug. ***
Comment 27 Elmar Ludwig 2005-10-31 12:52:31 PST
*** Bug 314533 has been marked as a duplicate of this bug. ***
Comment 28 Vlad Alexander 2005-10-31 13:09:12 PST
Here is another test-case:

http://xstandard.com/misc/mozilla/hide.htm

Web developers what to hide our plug-in like this:

document.getElementById('panel').style.display = 'none'

As soon as they do this, the instance of the plug-in is destroyed and all data in the plug-in is lost.
Comment 29 Phil Baines 2005-12-01 05:18:30 PST
I would like to back up Vlads comment. I am using multiple versions of  the xStandard plug-in on a page, where only one at a time gets displayed (for a multi-lingual CMS). Therefore we are using display:none; to hide panels. This is a necessary usability feature that we can not go back on. Unfortunately this means that we can not fully support Firefox within our website management system at the moment.

This bug may have gone relatively unnoticed until now, but with more and more web developers using DOM based JavaScript (and AJAX) to improve interface features this bug is going to mean that Firefox looses support in a lot of tomorrow’s web applications. 
Comment 30 Boris Zbarsky [:bz] 2005-12-01 09:20:25 PST
Guys, no need for advocacy. We know this needs fixing.  We're working on it.  If you have time to write some code, that would help, of course... ;)
Comment 31 Jiri Nemec 2006-03-08 23:15:43 PST
(In reply to comment #30)
> Guys, no need for advocacy. We know this needs fixing.  We're working on it. 
> If you have time to write some code, that would help, of course... ;)
> 

I would like to vote for this bug. I found this bug in Mozilla/5.0 (Windows; U; Windows NT 5.1; cs; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1.

getObject() function (see below) is my custom function:

function getObject(objectName){
  return document.getElementById(objectName);
}

This code works, it moves content from XStandard editor into hidden
file and vice versa:

// Only when XStandard is accessible
  if(getObject("xhtml1").value != undefined){
    if(tableId != "label"){
      // Set XStandard content into hidden file
      getObject("foo").value = getObject("xhtml1").value;
    }else{
      alert("Set XStandard content from hidden file");
      getObject("xhtml1").value = getObject("foo").value;
    }
  }

...and when I remove alert(); it doesn't work:

// Only when XStandard is accessible
  if(getObject("xhtml1").value != undefined){
    if(tableId != "label"){
      // Set XStandard content into hidden file
      getObject("foo").value = getObject("xhtml1").value;
    }else{
      getObject("xhtml1").value = getObject("foo").value;
    }
  }

It seems XStandard needs some delay to display its content.

This code sets content properly, but XStandard doesn't display it:

// Only when XStandard is accessible
  if(getObject("xhtml1").value != undefined){
    if(tableId != "label"){
      // Set XStandard content into hidden file
      getObject("foo").value = getObject("xhtml1").value;
    }else{
      getObject("xhtml1").value = getObject("foo").value;
      alert(getObject("xhtml1").value == getObject("foo").value);
    }
  }

An alert(); displays true, so content is setted properly but not
displayed. I found this behavior in Firefox, there is no problem in
Internet Explorer.

Mozilla/5.0 (Windows; U; Windows NT 5.1; cs; rv:1.8.0.1)
Gecko/20060111 Firefox/1.5.0.1
Comment 32 Frank Wein [:mcsmurf] 2006-05-29 05:26:34 PDT
*** Bug 339578 has been marked as a duplicate of this bug. ***
Comment 33 Marco Laponder 2006-05-29 05:58:58 PDT
This bug is pretty important for our application. I am a software developer myself with C++ knowledge , so if it is possible I would like to help solving this bug ( if someone can give me some pointers and some support ,and of course review my code ;-) ). Let me know if I can help solving this bug in any way.
Comment 34 Phil Baines 2006-06-08 03:11:35 PDT
(In reply to comment #33)
Marco, did you get any response to this? I'm not in a position to help you myself, as this is not my expertise. - I’m a front end and .net developer myself.

To all, I am just wondering if any progress has been made. This is a little hard to believe, but the comments on this bug (at least as it still is now) go back to 2002. That is 4 years ago! I guess it must be a really awkward bug.

In the mean time, has anyone been able to come up with a good work-around, specifically in relationship to the xStandard plug-in? See Vlad Alexander’s comments.

Best regards to all!
Phil Baines
Comment 35 Marco Laponder 2006-06-08 06:15:24 PDT
(In reply to comment #34)
Phil, I didn't get any reply to my comment. Right now I am working around the problemen by setting visibility to hidden en size to 0px. Not very neat but is does the trick in my situation. But I would like to really solve it instead working around it...
Comment 36 darrel 2006-06-08 10:38:39 PDT
> In the mean time, has anyone been able to come up with a good work-around,
> specifically in relationship to the xStandard plug-in? See Vlad Alexander’s
> comments.

Is this good? I don't know. But it is what we had to do to get around this bug. I agree, this is a rather important issue. It'd be great if xstandard could put some pressure towards this in hopes of getting it fixed.

Anyways, my solution:

Whenever I use javascript to hide the Xstandard editor, before hiding it, I first send the contents of the editor to a hidden form field value (the same one that you use to grab the content server-side):

document.getElementById('pageContent_textFromXlite').value = document.getElementById('editor1').value

Then whenever I use javascript to show the editor again, I check for this hidden field. If it's empty, then this would be the first time the editor is being 'shown' on the page and, as such, I shouldn't grab anything from the hidden field. If there IS content in the hidden field, then I should grab it, as it would be the more up-to-date copy of the content (compared to the variable grabbed from the DB on pageload)

if (document.getElementById('pageContent_textFromXlite').value != "") {
			document.getElementById('editor1').value = document.getElementById('pageContent_textFromXlite').value;
		}

Hope that helps.

Comment 37 Ross 2007-08-03 05:39:27 PDT
Darrel's comment from a year ago works, but has anyone found any new updates or workarounds?  Especially since everytime the content is hidden/destroyed, it reloads the applet forcing the user to wait for its startup time.  Has this bug really been open for 6 years?
Comment 38 Wayne Mery (:wsmwk, NI for questions) 2007-09-27 19:08:35 PDT
*** Bug 391015 has been marked as a duplicate of this bug. ***
Comment 39 Jesse Ruderman 2007-11-29 15:41:01 PST
*** Bug 406002 has been marked as a duplicate of this bug. ***
Comment 40 Elmar Ludwig 2008-01-07 12:17:38 PST
*** Bug 411149 has been marked as a duplicate of this bug. ***
Comment 41 Kevin McCoy 2008-01-09 12:37:37 PST
As the Dojo Toolkit gains popular use, this will become much more visible.   The Dojo toolkit uses style.display for containers such as the TabContainer and AccordionContainer when the appropriate node is deselected.   Typically one would expect to not lose state.  Such containers are often used to contain plug-ins (ex. FlashPlayer for Flash/Flex, Acrobat Reader).   This could force users to IE or other alternatives for affected applications.

So style.display ('none' or '') causes the problem, but the style.visibility attribute ('hidden' vs. 'visible') does not (plug-ins do not unload/reload) - am I missing a key distinction between the expected behavior (or implementation required in Firefox)?   Should style.display simply be handled the same as style.visibility?
Comment 42 Matt 2008-01-26 02:53:46 PST
*** Bug 414029 has been marked as a duplicate of this bug. ***
Comment 43 Matt 2008-01-26 03:12:29 PST
Workaround for previous tag changes only - 
If you can enclose the previous tag in a block, which does not change, the
object will not re-render. You can style the block to float:left to give the
effect of display:inline.
This does not help with display:none/block/inline for the object though
Comment 44 Steve Roussey (:sroussey) 2008-04-09 18:56:26 PDT
All charting apps that use flash have this reload problem (hiding/showing a FusionChart, for example, reloads the data and restarts the animation). Also seen with design mode iframes for WYSIWYG editors. Usually these effects can be seen with any JS UI lib that allows animation and hiding/showing of divs (as windows, accordions, etc.).
Comment 45 Tristan Schmelcher 2008-06-19 18:07:47 PDT
Hi. I'm an NPAPI plugin developer and I encountered this unload/reload bug when debugging my plugin with the final FF3 release. Long story short, I was able to locate the place in the code where the bug happens. It's in nsObjectLoadingContent (content/base/src/nsObjectLoadingContent.cpp). The HasNewFrame method (called during reflow) schedules an event to asynchronously instantiate the plugin, but it does this even if the plugin has already been instantiated! Then when the event runs, nsObjectFrame (layout/generic/nsObjectFrame.cpp) gets told to instantiate an already-instantiated plugin and it destroys the existing instance first (in nsObjectFrame::PrepareInstanceOwner).

I have attached a patch made against FF3 that seems to fix the bug (at least on my one machine and for my one plugin). I simply changed nsObjectLoadingContent::HasNewFrame into a no-op. There are comments there suggesting that someone was supposed to do that already (they even refer to this very bug number), which is what suggests to me that this may be the correct fix. I'm not a Mozilla developer though and I don't fully understand this code so someone needs to take a closer look.
Comment 46 Tristan Schmelcher 2008-06-19 18:11:15 PDT
Created attachment 325884 [details] [diff] [review]
Proposed fix for the plugin unload/reload bug

Turns nsObjectLoadingContent::HasNewFrame into a no-op, instead of asynchronously (re-)instantiating the plugin. Needs attention from an expert in this area of the code.
Comment 47 Tristan Schmelcher 2008-06-19 18:15:10 PDT
Oh, I forgot to mention: if you're a webpage developer then another possible workaround is to call some method on the plugin's DOM element (or maybe just dereference a property?) right after any action that would normally trigger this bug. Doing that causes nsObjectLoadingContent::EnsureInstantiation to run, which as a side-effect will cancel the pending event that is responsible for the bug.
Comment 48 Tristan Schmelcher 2008-06-23 15:40:45 PDT
After some more testing, it looks like HasNewFrame has to do what it does in order for plugins to load on their own without waiting for something else to force them to, so the above patch is no good. It looks like my plugin's problems can be fixed by making HasNewFrame check to see if the plugin is already instantiated for that frame, which fixes an instantiation race when the instantiation gets done synchronously before the HasNewFrame() call occurs (which used to cause an unload/reload). However, that doesn't fix the problem with style.display = 'none'. I've attached a new patch to fix the race, but it should maybe be moved to a separate bug.
Comment 49 Tristan Schmelcher 2008-06-23 15:46:49 PDT
Created attachment 326389 [details] [diff] [review]
Don't asynchronously instantiate in HasNewFrame if already instantiated for that frame. Fixes a race where EnsureInstantiation would instantiate the plugin and then HasNewFrame would re-instantiate it
Comment 50 Johnny Stenback (:jst, jst@mozilla.com) 2008-06-26 10:00:28 PDT
Tristan, I think your problem is also covered by bug 438830. And note that this bug is not fixed, the part about moving plugins to content. We're part of the way there, but our layout code, as opposed to our content code, is still required for plugin loading, and that's what we're hopefully going to fix in this bug, at some point. Can you test if the patch in bug 438830 fixes the issue you're seeing?
Comment 51 Tristan Schmelcher 2008-06-26 10:37:51 PDT
Thanks Johnny, you're right, bug 438830 is the problem I'm seeing. Looking at the patch there, I don't even need to test it out, it's basically the same as mine. :) I'll post my findings there.
Comment 52 Phillip Whelan 2008-08-25 13:44:58 PDT
All of this seems very closely related to <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=451816">Bug 451816</a>  It seems manipulating a number of different styles causes the plugin to reinitialize.  I am seeing the issue in 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.16) Gecko/20080702 Firefox/2.0.0.16

and 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

I am a web developer working with flash embedded as a component where we need to maximize/minimize, drag/drop. This bug is causing no end of head aches and currently its manifesting itself in the browser completely crashing, sometimes with out warning.  We've traced it to place where we modify the overflow and position of the container element of the flash.
Comment 53 Johnny Stenback (:jst, jst@mozilla.com) 2008-08-25 22:03:11 PDT
Phillip, if you're able to reproduce any of those crashes please do file new bugs with steps to reproduce the crashes. The crashes would be dealt with separately from this bug more than likely. Thanks.
Comment 54 Phillip Whelan 2008-08-26 06:10:57 PDT
I will try and create a simple test case and open a new bug for the crash.  What I was really trying to get across however is that the re-frame causing the flash component to be reset is still occurring in 3.01, and from what i understand it was believed to be fixed in the bug 438830 referenced above.
Comment 55 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-18 15:05:53 PDT
*** Bug 451816 has been marked as a duplicate of this bug. ***
Comment 56 Zack Weinberg (:zwol) 2008-10-20 17:34:00 PDT
> What I was really trying to get across however is that the re-frame causing the
> flash component to be reset is still occurring in 3.01, and from what i
> understand it was believed to be fixed in the bug 438830 referenced above.

I'm afraid not :-(  Bug 438830, if I understand correctly, was only about a plugin getting loaded twice *during page load*, under certain conditions.
Comment 57 Rupert Carr-Smith 2008-11-03 13:32:41 PST
Do anyone know if this bug will be fixed. Our app sometimes hides and shows divs with occassionally a plug in (eg flash / xstndard) and when the user returns the content hqas gone.
Comment 58 Zack Weinberg (:zwol) 2008-11-03 13:38:52 PST
(In reply to comment #57)

I'm not promising anything, but it is my intention to see this fixed in the next release cycle (not 3.1).
Comment 59 Jesse Ruderman 2009-02-26 01:45:32 PST
*** Bug 479807 has been marked as a duplicate of this bug. ***
Comment 60 Simon Charette 2009-03-20 12:32:56 PDT
This one is a real bugger. We're using an embeded flash to handle upload with AS3 FileReference and when we lauch the dialog (hidding body overflow with document.body.style.overflow = 'hidden') showing download progress we reload the flash so we lose the FileReference and we get an upload failure since no files are selected anymore. Any updates on a patch release?
Comment 61 Simon Charette 2009-04-28 08:09:38 PDT
Is there any progress on this issue?
Comment 62 djkrogh 2009-05-13 09:14:38 PDT
Any news on this bug? Really annoying...
Comment 63 Arseniy Terekhine 2009-07-29 07:35:10 PDT
This is a must-fix bug! But all i have is hope.
Comment 64 Kenneth Russell 2009-07-29 16:01:28 PDT
*** Bug 507182 has been marked as a duplicate of this bug. ***
Comment 65 Emil Ivanov 2009-10-02 08:35:35 PDT
*** Bug 262354 has been marked as a duplicate of this bug. ***
Comment 66 Josh Heidenreich 2009-11-03 22:08:26 PST
Is there anything major that is holding this bug up?

Does the patch provided by Tristan Schmelcher work, and if it does, can this bug be fixed?
Comment 67 Tristan Schmelcher 2009-11-04 04:33:31 PST
No, my patch was actually for fixing bug 438830 in FF3, I just mistakenly posted it here. I believe 438830 has since been fixed, so you can disregard my patch.
Comment 68 Phillip Whelan 2009-11-06 12:12:26 PST
Created attachment 410848 [details]
test page to display flash reset problem

I am not sure if this covered by another bug or not, I haven't been able to find one if it is.

In addition to toggling the display property from none to block, changing the overflow property from "" to hidden, scroll, auto all cause flash to reset.  Also changing the position property from static to absolute and back again will cause the reset.

I am attaching a zip file with a simple html file that will display these behavior.  I've tested this page across Firefox 3.5.4 rv1.9.1.4, Chrome, IE7 and Safari.

IE7 does not reset the flash object at all when manipulating the style properties.
Chrome and Safari both reset the flash object when the display is set to none.

Firefox resets in all the scenarios.
Comment 69 Phillip Whelan 2009-11-06 12:27:40 PST
Comment on attachment 410848 [details]
test page to display flash reset problem

Removing and adding a better test case.
Comment 70 Phillip Whelan 2009-11-06 12:28:28 PST
Created attachment 410850 [details]
Page showing the flash reset problem
Comment 71 Matthew W 2009-11-11 07:35:34 PST
Another workaround which might help some people in some cases:

Try making your flash movie a direct child of document.documentElement (ie the <html> element)

Firefox seems to allow this, and then you can get away with changing the 'overflow' styling on body without restarting the flash movie.

This works ok for us because the flash movie is just an invisible flash bug to expose flash APIs to javascript. So it doesn't really matter to us where in the DOM it lives provided it works. That said FF does still seem to render it.
Comment 72 Emil Ivanov 2010-01-13 17:29:22 PST
*** Bug 303888 has been marked as a duplicate of this bug. ***
Comment 73 Johnny Stenback (:jst, jst@mozilla.com) 2010-01-26 18:04:56 PST
biesi, you took this bug, do you think you're likely to work on it in the foreseeable future?
Comment 74 Loque 2010-03-01 00:53:26 PST
Has to be said, viewing how this bug has been handled is a real eye opener. Its a complete blocker and an issue that only FF suffers from that has, and will hold back many web development projects, including the one I am working on... and here I am reading this log... 9 years guys, ... wow, let me guess, the next guy that takes this one also thinks its an easy bug to fix? Or you just dont have the time to do another re-write of something because you have to roll a release out...  Ye, I've been there, push-the-heck-back and get the product right! And delete this comment. :`x
Comment 75 Loque 2010-03-01 02:06:11 PST
Just in-case this is of any help, here is the W3C spec FF is conforming too a little too tightly? I really wish I could help!

http://www.w3.org/TR/CSS2/visuren.html#block-formatting
Comment 76 ERaos 2010-04-09 06:07:33 PDT
Hi guys.

First of all would like to thank everyone for all the hard work in making of this browser. It really is state of the art.
But unfortunately as we all know every soft has it's own bugs. This bug in particular affects my daily work since I'm involved in moving containers in dom hierarchy and changing their properties constantly on other vendor websites and as a rule they always contain flash which makes them reset every time I touch them :)

Since I don't have access to source files of these websites you can imagine my luck when I need to change position, display or overflow on container using js :P

Is there a chance this would ever be fixed? It would make my day, literally.
In the mean time keep up the good work.

Cheers,

Ed
Comment 77 Michelle Xing 2010-04-09 06:08:52 PDT
I got a bug from my user, and we did some investigation then find out it's problem of firefox. Could you please fix it, or give us a solution?
Here is my example help to reproduce the issue:
  Embedded a object which contain Time information in the html page, like:
<html lang="en">
<head>
<title>Test page</title>
<script>
var test = {		
   //only for firefox/chrome
   hideVerticalScrollBar : function() {
      window.document.childNodes[0].style.overflowY = 'hidden'
   },
   showVerticalScrollBar : function() {
      window.document.childNodes[0].style.overflowY = 'scroll'
   }
}
</script>
</head>

<body onLoad="alert('hello world!')">
<div id="1">
	<input value="show scrollbars" onclick="javascript:test.showVerticalScrollBar()" type="submit"/>
	<input value="hide scrollbars" onclick="javascript:test.hideVerticalScrollBar()" type="submit"/>
</div>
<OBJECT CLASSID="java:Reload.class" CODETYPE="application/java" WIDTH=400 HEIGHT=250>
</OBJECT>
</body>
</html>

And in the Reload.java:
public class Reload extends Applet{
	@Override
	public void init(){
		super.init();
	}
	public void paint(Graphics g){
	    g.drawString(new java.util.Date().toString(),40,20);
	}
	public void start(){
		super.start();
	}
}

Please run the example in Firefox, you’ll find there is an alert popup when the page is loaded. And no matter how many time you click the button, the alert will not show up again. It means Firefox doesn’t reload the whole page but only the “object”. Am I right?

Then you'll find out the time is changed in the page, but hidden the scroll bar why the object need be repainted?
Comment 78 Si Forster 2010-04-13 08:50:19 PDT
(In reply to comment #74)
> Has to be said, viewing how this bug has been handled is a real eye opener. Its
> a complete blocker and an issue that only FF suffers from that has, and will
> hold back many web development projects, including the one I am working on...
> and here I am reading this log... 9 years guys, ... wow, let me guess, the next
> guy that takes this one also thinks its an easy bug to fix? Or you just dont
> have the time to do another re-write of something because you have to roll a
> release out...  Ye, I've been there, push-the-heck-back and get the product
> right! And delete this comment. :`x

Agreed completely. What's going on with this bug? 9 years is a little epic don't ya think?!
Comment 79 Phillip Whelan 2010-04-13 10:13:49 PDT
This bug has been on my wishlist of firefox fixes for a number of years now.

Could we at least get a status as to what's going on with this bug, why it hasn't been fixed? Is there anything the community can do to help get it on its way?

This is a major headache when you are trying to use flash as a component in a web app.
Comment 80 Zack Weinberg (:zwol) 2010-04-13 11:04:34 PDT
I have made a couple of attempts to fix this bug in the past year.  I'll try to outline what the situation is, and why you should not expect it to be fixed soon.

When you load an HTML page, Gecko (that is, Firefox's rendering engine) builds two internal data structures.  One is called the "content tree"; it is a very close analogue of the HTML DOM tree.  There is a C++ object in there that directly represents the <embed> or <object> element.  The other is called the "frame tree"; it corresponds roughly to the CSS box tree.  (The analogy is much looser for the frame tree than the content tree.)  The content tree exists for as long as the page is displayed, sometimes longer (it might get cached in back/forward history) but the frame tree (or bits of it) get torn down and re-created whenever that's the easiest way of updating it for a dynamic page change.  There is *not* necessarily a box object in the frame tree for every element in the DOM -- for instance, elements with display:none do not contribute to the frame tree.

The cause of this bug is that all of the plugin's live state is attached to the plugin's box in the frame tree rather than the DOM element, for historical reasons (it used to be that every active plugin had to have an operating-system-level nested window associated with it, and those have to be tracked by the frame tree).  When you change display: or overflow: or several other properties on an <object>/<embed>, the plugin's box is destroyed (possibly to be re-created immediately, possibly not) and the plugin's live state is lost with it.

So why don't we just move the plugin state to the DOM element?  Well, that's what I've tried to do, twice now, and I'm here to tell you that it is really damn hard, because there are many places in the code that have expectations of the plugin state.  They expect to be able to get at it from the box tree.  They expect that it doesn't exist when there is no box.  They expect to be able to create fake plugin states when they have no access to the content tree.  Et cetera.  If we managed to make all the necessary changes to *our* code, we would still have to do extensive testing to make sure that we weren't breaking any *plugins'* expectations as well.

I still want to see it done, but there is always something for me to do that is both easier and has greater utility (i.e. improves Firefox more, for more people, than fixing this bug would).

If any of you would care to step up to the plate, start by reading all the code in this file: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp>.  The necessary internal change is to move the nsPluginInstanceOwner object from there to an association with this class: <http://mxr.mozilla.org/mozilla-central/ident?i=nsObjectLoadingContent>
Comment 81 Emil Ivanov 2010-04-13 12:04:57 PDT
*** Bug 340465 has been marked as a duplicate of this bug. ***
Comment 82 Emil Ivanov 2010-04-13 12:31:20 PDT
(In reply to comment #80)

> I still want to see it done, but there is always something for me to do that is
> both easier and has greater utility (i.e. improves Firefox more, for more
> people, than fixing this bug would).

Maybe resolving this bug is not easier but that doesnt mean it shouldnt be fixed. This bug is very serious thing and 9 years are too long. Its true that there are things which can bring more functionality in the browser but bugs have to be fixed not ignored _forever_.

There will be always more important things than this, if we think that way it will never be fixed...

I will share one story after which I found this bug. A client wanted to redesign his site and it was for flash games, there was a functionality to resize the game to fit your desired view. I decided that it will be nice to make this smooth with animation, so I have used jQuery animations. Every time this functionality was used and the game was restarted from the beginning. Now the site animates in all browsers except Firefox because it is _destroying_ the games. At the time of the redesign the site had around 10 000 visitors per day now they are more but at this moment I dont have stats for it.
Comment 83 Zack Weinberg (:zwol) 2010-04-13 12:47:39 PDT
(In reply to comment #82)
> > I still want to see it done, but there is always something for me to do that is
> > both easier and has greater utility (i.e. improves Firefox more, for more
> > people, than fixing this bug would).
> 
> Maybe resolving this bug is not easier but that doesnt mean it shouldnt be
> fixed. This bug is very serious thing and 9 years are too long. Its true that
> there are things which can bring more functionality in the browser but bugs
> have to be fixed not ignored _forever_.

The "has greater utility" part of what I said is more important than the "easier" part.  We do understand that this is a serious problem that hurts many websites, but in the past two years I've been working on Firefox there has _always_ been at least one other bug that I could be working on, that is a _worse_ problem and hurts _more_ websites.  That's why this goes unfixed for nine years.
Comment 84 Boris Zbarsky [:bz] 2010-04-26 13:25:56 PDT
*** Bug 502753 has been marked as a duplicate of this bug. ***
Comment 85 Boris Zbarsky [:bz] 2010-04-26 13:26:01 PDT
*** Bug 507715 has been marked as a duplicate of this bug. ***
Comment 86 Alexander Farkas 2010-04-27 04:01:29 PDT
@Zack Weinberg

Sorry, your explanation is a little bit stupid. Mozilla has worked on a lot of features in past year. Most of them can´t be used today. Instead of adding plenty of less important features, it would be great, if you start fixing this extremly annoying bug. With a philosophy, like yours, firefox gets more buggy and unstable with every new version.

This is the cause, for a lot people to move to alternate browsers like chorme.
Comment 87 Johnny Stenback (:jst, jst@mozilla.com) 2010-05-20 12:55:50 PDT
Reassigning to Justin who will spend time on this bug this coming summer. I'm not sure this will end up being something we can hold 1.9.3 for, but if we have a patch with enough certainty early enough, I'd love to see this go in for 1.9.3.
Comment 88 Justin Lebar (not reading bugmail) 2010-07-09 01:09:55 PDT
Reassigning to nobody. Per Shaver's talk tonight, this can't be a priority right now.
Comment 89 Phillip Whelan 2010-07-09 06:20:55 PDT
This can't be a priority right now?  Is this a veiled attempt at discouraging the use of flash in Firefox?  This bug is 9 years old! If it's not a priority now when exactly will it be a priority.

Pretty disappointing...
Comment 90 Michael A. Puls II 2010-07-09 06:26:07 PDT
(In reply to comment #89)
> This can't be a priority right now?  Is this a veiled attempt at discouraging
> the use of flash in Firefox?  This bug is 9 years old! If it's not a priority
> now when exactly will it be a priority.
> 
> Pretty disappointing...

Well, HTML5 says this needs to be done. So, some day.
Comment 91 Ryan Hanna 2010-07-09 09:41:33 PDT
(In reply to comment #88)
> Reassigning to nobody. Per Shaver's talk tonight, this can't be a priority
> right now.

Justin, can you please provide more details why this bug is not considered a priority?
Comment 92 Justin Lebar (not reading bugmail) 2010-07-16 16:27:19 PDT
(Sorry for the delay in responding -- I didn't realize that unassigning myself from the bug would take me off the cc list.)

I understand that this is a really painful bug; it's disappointing to me too that we can't work on it right now.  But our focus for Firefox 4 is on performance, and we're dropping a number of fixes and enhancements so that we can meet our speed goals.  (Mike Shaver, our VP of Engineering, presented this plan at the summit last week -- this was the talk I cryptically alluded to above.)

I realize that my speeding up Firefox won't improve web developers' lives to nearly the same extent as my fixing this bug, so I hope we'll be able to revisit this issue soon.
Comment 93 Benjamin Smedberg [:bsmedberg] 2010-07-20 13:03:40 PDT
I think our decision on whether this bug is a blocker needs to be determined by app-tabs UI decision. If we're going to share a single app-tab among all Firefox windows, then we really need to solve this bug, since common app-tabs like gmail use plugins behind the scenes.
Comment 94 TJ 2010-08-17 07:49:24 PDT
Hi 

I am seeing this issue on the website of a client where the Flash container is being moved to make space for another container. I can understand that Firefox 4 has a higher priority than a number of fixes for Firefox 3, however the current Firefox 4 beta also has this problem. Hopefully this issue will be addressed soon, since Firefox 3 and 4 beta seem to be the only browsers with this problem.

Kind Regards
tj
Comment 95 Zack Weinberg (:zwol) 2010-08-17 09:29:21 PDT
(In reply to comment #93)
> I think our decision on whether this bug is a blocker needs to be determined by
> app-tabs UI decision. If we're going to share a single app-tab among all
> Firefox windows, then we really need to solve this bug, since common app-tabs
> like gmail use plugins behind the scenes.

This is at least a man-month of effort, so if it's going to block, someone needs to be working on it *now*.
Comment 96 Benjamin Smedberg [:bsmedberg] 2010-08-18 06:38:00 PDT
Per the platform meeting, *this* bug is not going to block, but bug 449734 does so that app tabs and tabcandy and such work.
Comment 97 Séamus O'Connor 2010-09-01 16:53:02 PDT
*** Bug 429428 has been marked as a duplicate of this bug. ***
Comment 98 Boris Zbarsky [:bz] 2010-09-06 19:36:45 PDT
*** Bug 590891 has been marked as a duplicate of this bug. ***
Comment 99 Josh Aas 2010-09-14 14:47:57 PDT
I'm planning to work on this for after Gecko 2.0.0.
Comment 100 Yuval Adam 2010-09-27 07:01:01 PDT
This is by far the most epic bug I've encountered.

Judging by the way this bug was handled so far, I'm not optimistic that we'll see any advance any time soon. So I propose the following:

Is it possible for some key players (:zwol, :jlebar, :jst) to prepare a technical overview of the relevant areas in the code (https://bugzilla.mozilla.org/show_bug.cgi?id=90268#c80 is a good start) so that a group of developers can start tackling this seriously?

I, for one, would love to see this bug resolved, and have no problem devoting the time to tackle it - given adequate support from the mozilla team.
Comment 101 Zack Weinberg (:zwol) 2010-09-27 08:07:41 PDT
(In reply to comment #100)
> Is it possible for some key players (:zwol, :jlebar, :jst) to prepare a
> technical overview of the relevant areas in the code
> (https://bugzilla.mozilla.org/show_bug.cgi?id=90268#c80 is a good start) so
> that a group of developers can start tackling this seriously?
> 
> I, for one, would love to see this bug resolved, and have no problem devoting
> the time to tackle it - given adequate support from the mozilla team.

Unfortunately, the amount of work you're asking for, from us, is not significantly less than the amount of work it would take to fix the bug :(  A large part of the problem is that this is old code that is not well understood by anyone who's still with the project.

I can tell you that the relevant classes are: nsObjectFrame (and everything else in layout/generic/nsObjectFrame.cpp, especially nsPluginInstanceOwner); nsObjectLoadingContent; nsHTMLObjectElement; and everything below modules/plugin, particularly nsPluginInstance and nsPluginHost.  The last time I looked at this was before layers and electrolysis, so there's probably additional complications I don't know about.

I can try to answer specific questions if you have them (by email, perhaps, rather than here).  I recommend that you file new bugs for specific changes as you identify them, and make them block this bug.
Comment 102 Boris Zbarsky [:bz] 2010-11-15 11:25:22 PST
*** Bug 488167 has been marked as a duplicate of this bug. ***
Comment 103 noitidart 2011-01-26 18:10:20 PST
Hoping this gets fixed some day. :(
Comment 104 astgtciv 2011-02-01 11:02:37 PST
Wow! The only FF bug of similar epic-ness was that US-keyboard layout forced in Flash embedded with wmode=transparent. And that was fixed after 10 years or so - so there is still hope!

I tried the workarounds suggested above but couldn't get anything to work (i didn't try the "child-of-html" from Comment 71 because it doesn't apply to my situation). So - is there a way to work around this from javascript somehow? A real workround would be worth almost as much as this bug being fixed to me...
Comment 105 Boris Zbarsky [:bz] 2011-04-04 23:37:58 PDT
*** Bug 647739 has been marked as a duplicate of this bug. ***
Comment 106 Abi 2011-04-27 21:41:49 PDT
Our ERP system is getting resigned from Oracle 10g forms to New Web interface. The forms section still there at least 70% now, and it's intimately working inside applet. It is an internal application and used above 2000 users. 

So far our default browser for another application is Mozilla only. 

In the new application, for client side we are using GWT.  

We are calling the Forms inside a Tab. So have 4-5 tabs. But the issue is when we switch from one tab to another tab, the forms are getting reloading, not maintaining the state, which is killing our server as each time, it is creating a process if  the forms get reloaded and too slow. 

Basically the Form applet can't remember the state. But we checked Google Chrome and I.E, but they are keeping the applet state. 

But some other reasons we like Mozilla the most as the no. of options & the css styles rendering provided by Mozilla is great. 

So we are waiting to get this bug fixed so we still can continue with FF.
Comment 107 alex 2011-05-17 07:36:49 PDT
Well I for one, encountered this problem too, and it occurs when the body has a overflow hidden, removing this, fixes the problem for me. Use this structure:

<div>
    <embed style="width: 100%; height: 100%">
</div>

And resize the div, don't use strange positioning and dont attach overflow to the body.

By the way, if the firefox dev team continues this way, you will end up with Firefox 7, that is very fast, but has a lot of bugs and eventually displays only white pages.

This bugzilla report is the reason I moved to Google Chrome, it's just plain old crazy that a bug is around for 9 years, and nobody seems to want to fix it.
Comment 108 Zack Weinberg (:zwol) 2011-05-17 07:43:35 PDT
This cannot block.  It is multiple man-months of effort to fix and nobody has stepped up.
Comment 109 Josh Aas 2011-05-17 07:52:41 PDT
I am getting close to beginning work on this.
Comment 110 Josh Aas 2011-05-21 11:34:51 PDT
I am starting work on this today.
Comment 111 AndreiD[QA] 2011-05-23 05:04:47 PDT
*** Bug 658980 has been marked as a duplicate of this bug. ***
Comment 112 rob 2011-05-26 10:51:21 PDT
Go Josh! I hate to pile onto the heated priority debate, but it needs to be pointed out that Facebook has basically become unusable in FF now (see bug 643683). That's a whole lotta users to lose...
Comment 113 Phil Baines 2011-05-27 01:26:50 PDT
Almost ten years now! 

"A large part of the problem is that this is old code that is not well understood by anyone who's still with the project." - and that's why periodically refactoring code is a good idea!
Comment 114 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-06-03 15:31:39 PDT
*** Bug 661950 has been marked as a duplicate of this bug. ***
Comment 115 Josh Aas 2011-06-08 23:31:32 PDT
Quick update: I've made a lot of progress, but there is a lot of work left to do. Most previous capabilities are working again after the ownership change, and the display:none case works now, but re-framing isn't quite there and going into the bfcache messes up npruntime support. I've been focusing on Mac OS X, I haven't touched things specific to Windows or Linux yet. Should have something good enough to post in a week or so. I doubt I'll have something ready for review until the end of June.
Comment 116 Josh Aas 2011-06-09 22:41:54 PDT
Created attachment 538440 [details] [diff] [review]
fix v0.4

Posting this as public backup of my work so far. This will only compile on Mac OS X. Things work pretty well with this patch, including all basic web content I tried (YouTube, Hulu, ESPN). Plugin instances that are display:none work well now too (see included mochitest). Reframing is not quite there yet (I haven't dealt with widget ownership properly yet). The bfcache stuff I mentioned before is taken care of, so npruntime is fully functional now.
Comment 117 Vitali Kupets 2011-06-15 04:51:02 PDT
good to see there's some progress. Too bad the timing is not quite right for me and it looks like we are going to lose our client, as he isn't willing to dispense with messing around with documentElement overflow.
Comment 118 Josh Aas 2011-06-15 07:54:24 PDT
Created attachment 539517 [details] [diff] [review]
fix v0.6

Adds support for Linux, and re-framing works now.
Comment 119 :Ehsan Akhgari (busy, don't ask for review please) 2011-06-15 11:43:27 PDT
Created attachment 539607 [details] [diff] [review]
fix v0.6 + windows build fix

This version of the patch builds successfully on Windows too.
Comment 120 Josh Aas 2011-06-16 00:01:32 PDT
Created attachment 539735 [details] [diff] [review]
fix v0.7

Thanks for the Windows build fix Ehsan. Your version is better than what I had come up with. An event handling bug in my patch prevented the resulting Windows build from working properly, this updated patch includes a fix.

At this point most things work pretty well on Windows, Mac, and Linux. Most tests pass too, and the ones that don't have a known cause. They depend on plugin instances getting destroyed when their node is disconnected from the DOM, which doesn't happen with my patch. I have a solution which I'll post next.

One I have all tests passing I still need to go through this patch and address some comments I left for myself and generally clean things up before I request review from others. I'm pushing this patch to the holly branch tonight so that people can play with the resulting builds.
Comment 121 AndreiD[QA] 2011-06-22 05:25:59 PDT
*** Bug 665975 has been marked as a duplicate of this bug. ***
Comment 122 XtC4UaLL [:xtc4uall] 2011-06-22 16:04:21 PDT
*** Bug 666366 has been marked as a duplicate of this bug. ***
Comment 123 Josh Aas 2011-06-25 15:57:17 PDT
Created attachment 541965 [details] [diff] [review]
fix v0.75

This has a crash fix, a memory leak fix, test fixes, and it supports destroying an instance by removing a DOM node from its document and leaving it unattached until control returns to the runloop. It regresses windowed plugins on Windows and Linux (they don't work at all). Windowless plugins should work just fine on Windows and Linux.
Comment 124 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-26 19:17:06 PDT
(In reply to comment #120)
> At this point most things work pretty well on Windows, Mac, and Linux. Most
> tests pass too, and the ones that don't have a known cause. They depend on
> plugin instances getting destroyed when their node is disconnected from the
> DOM, which doesn't happen with my patch.

Do other browsers do this? What exactly is the behavior of other browsers?
Comment 125 Karl Tomlinson (ni?:karlt) 2011-06-26 20:04:41 PDT
I gather the issue with Linux (and Windows?) windowed plugins is that creating an nsIWidget (with a native window) fails when attempted with a null parent.
That would be mostly due to our widget code, rather than GDK itself.

I expect it's going to be a bit awkward and inefficient (because of our widget code, mostly) to create a widget with a null parent and then later provide the
parent.

And I don't think a plugin instance needs a window unless it is going to
draw.  If it doesn't have a frame, then we don't even know what
size to make the window.

So, can the widget be created on demand when we have a frame?

I expect keeping the window from one frame to another is probably going to be fairly straightforward, assuming plugins don't get too surprised - I assume we already do something similar when dragging a tab from one window to another.  The difference will be that the widget will have a null parent for some period of time, but I don't imagine that being difficult to implement.

The creation of a widget without a parent is the awkward part.
I expect code can be added to handle that but I'd rather not do that unnecessarily, because creating the widget lazily seems the better option.
Comment 126 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-26 20:11:10 PDT
(In reply to comment #125)
> I expect keeping the window from one frame to another is probably going to
> be fairly straightforward, assuming plugins don't get too surprised - I
> assume we already do something similar when dragging a tab from one window
> to another.

No, we preserve the frame in that case.

I'm skeptical that plugins will cope well with not having a widget, if they've never had to cope with that before. Do other browsers give display:none plugins a widget?

What's the problem with given frameless plugins a widget whose parent is the browser window's widget? These days that is the only parent a plugin widget can ever have.

Yes, we'd have to make up a size for a frameless plugin's widget, but that's OK IMHO.
Comment 127 Karl Tomlinson (ni?:karlt) 2011-06-26 20:29:09 PDT
(In reply to comment #126)
> I'm skeptical that plugins will cope well with not having a widget, if
> they've never had to cope with that before.

I would also be skeptical about giving plugins a null native window, but the intention is to keep the window from one frame to the next.  Our nsIWidget would get reparented (which we do from nsObjectFrame::EndSwapDocShells with tab dragging) and would temporarily have a null parent.

> What's the problem with given frameless plugins a widget whose parent is the
> browser window's widget?

I guess that's possible, better than a null widget, thanks, but I'm not sure its ideal.

> Yes, we'd have to make up a size for a frameless plugin's widget, but that's
> OK IMHO.

It would mean an additional setwindow with the wrong size.  Last I checked flash player recreated its child window on change in size, which may flicker.
Comment 128 Josh Aas 2011-06-26 20:29:57 PDT
I agree that plugins are unlikely to be happy about not having a widget even in the display:none case. Also, a re-parent involves disconnecting the widget from an object frame and then connecting it to a new frame when one comes along, which it might not. So the re-parent case is like the pre-frame case in that the plugin will exist for a certain time without a frame. It's not a simple swap from one frame to another, it's parent->null, null->parent. If necessary we might be able to keep the widget parented and just tell it that it's entirely clipped, or whatever we do when another tab covers it.

I haven't paid much attention to the issue of how we report a widget's size to a plugin when there is no frame. Hasn't been a problem yet but it's on my to-do list.
Comment 129 Josh Aas 2011-06-26 20:33:33 PDT
(In reply to comment #124)
> (In reply to comment #120)
> > At this point most things work pretty well on Windows, Mac, and Linux. Most
> > tests pass too, and the ones that don't have a known cause. They depend on
> > plugin instances getting destroyed when their node is disconnected from the
> > DOM, which doesn't happen with my patch.
> 
> Do other browsers do this? What exactly is the behavior of other browsers?

I haven't looked at what other browser do yet but the behavior in my current patch is the only reasonable thing I could think of. The instance is stopped if its DOM node isn't re-parented before the browser returns to the event loop. I will look into the behavior of other browsers soon.
Comment 130 Karl Tomlinson (ni?:karlt) 2011-06-26 20:37:30 PDT
(In reply to comment #127)
>  and would temporarily have a null parent.

Actually, yes, it doesn't need to have a null parent, assuming we know the browser window.
Comment 131 Karl Tomlinson (ni?:karlt) 2011-06-26 20:59:33 PDT
... and we know that the plugin instance will be destroyed or reparented before the browser native window is destroyed.
Comment 132 Vlad Alexander 2011-06-26 21:39:03 PDT
Could you please include XStandard WYSIWYG edtior plug-in in your testing. It's a windowed plug-in.

For Windows:
http://xstandard.com/download/NPXStandard.dll

For OS X 10.6:
http://xstandard.com/download/x-lite.dmg

For OS X 10.5:
http://belus.com/misc/dev/41/x-lite.dmg


Test page:
http://xstandard.com/misc/mozilla/hide.htm
Comment 133 Josh Aas 2011-06-27 06:47:37 PDT
Created attachment 542143 [details] [diff] [review]
fix v0.75 updated to latest m-c
Comment 134 j.j. 2011-06-30 07:21:01 PDT
*** Bug 667213 has been marked as a duplicate of this bug. ***
Comment 135 Josh Aas 2011-07-11 10:43:18 PDT
Created attachment 545206 [details] [diff] [review]
fix v0.76

Fixes a memory leak, updates to latest m-c.
Comment 136 Josh Aas 2011-07-13 22:43:56 PDT
Created attachment 545832 [details] [diff] [review]
fix v0.8

Makes windowed plugins work on Linux and Windows. Fixes a number of test failures.
Comment 137 Josh Aas 2011-07-22 05:44:32 PDT
Created attachment 547669 [details] [diff] [review]
fix v0.8, updated to current m-c
Comment 138 XtC4UaLL [:xtc4uall] 2011-07-29 18:52:03 PDT
*** Bug 670729 has been marked as a duplicate of this bug. ***
Comment 139 Mardeg 2011-07-31 19:58:15 PDT
*** Bug 675557 has been marked as a duplicate of this bug. ***
Comment 140 ppju 2011-07-31 22:20:37 PDT
I have some questions, hoping to get the answer.

1. When will this bug be fixed completely?
2. Do you have the schedule for this bug? 
3. which firefox version will this bug go into?

Thank you.
Comment 141 Josh Aas 2011-08-09 09:02:54 PDT
Created attachment 551786 [details] [diff] [review]
fix v0.8, updated to current m-c
Comment 142 Josh Aas 2011-08-19 14:09:16 PDT
Created attachment 554544 [details] [diff] [review]
fix v0.8, updated to current m-c
Comment 143 Josh Aas 2011-08-21 15:09:18 PDT
Created attachment 554760 [details] [diff] [review]
fix v0.82

Includes a fix for some test failures. Still have to resolve more test failures.
Comment 144 Josh Aas 2011-08-22 17:24:05 PDT
Created attachment 554998 [details] [diff] [review]
fix v1.0
Comment 145 Josh Aas 2011-08-22 17:36:21 PDT
Comment on attachment 554998 [details] [diff] [review]
fix v1.0

I'm not done looking for problems with this patch but there's nothing big that I know of remaining. Might as well start the review process.
Comment 146 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-22 22:00:23 PDT
Comment on attachment 554998 [details] [diff] [review]
fix v1.0

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

::: content/base/public/nsIObjectLoadingContent.idl
@@ +55,1 @@
>  interface nsIObjectLoadingContent : nsISupports

We should probably make this non-IDL some day...

@@ +94,5 @@
>     * because nsIObjectFrame is unscriptable.
>     */
>    [noscript] void hasNewFrame(in nsIObjectFrame aFrame);
>  
> +  [noscript] void DisconnectFrame();

disconnectFrame

::: content/base/src/nsObjectLoadingContent.cpp
@@ +479,5 @@
> +  if (!mInstanceOwner) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  nsIObjectFrame* frame = GetExistingFrame(eFlushLayout);

So, this only flushes layout if there isn't already an existing frame. Is that really what you want? Or should we do a proper layout flush here to ensure that the size is more likely to be correct?

@@ +500,5 @@
> +    objectFrame->FixupWindow(objectFrame->GetContentRectRelativeToSelf().Size());
> +    if (weakFrame.IsAlive()) {
> +      // Ensure we redraw when a plugin instance is instantiated
> +      objectFrame->Invalidate(objectFrame->GetContentRectRelativeToSelf());
> +    }

Can FixupWindow really destroy objectFrame? That seems surprising. If so, how? And what should happen in that case?

@@ +662,4 @@
>    // 2) Our type hint is a type that we support with a plugin.
> +  if (!mContentType.IsEmpty() &&
> +      ((channelType.EqualsASCII(APPLICATION_OCTET_STREAM) &&
> +        GetTypeOfContent(mContentType) != eType_Document) ||

Why have you changed the logic here?

@@ +924,2 @@
>  NS_IMETHODIMP
> +nsObjectLoadingContent::HasNewFrame(nsIObjectFrame* aFrame)

HasNewFrame really needs to be renamed now, because it's totally unclear how it differs from SetObjectFrame. In fact I'm not even sure why we need it. It gets called from DidReflow but we should already have set up the connection between the nsPluginInstanceOwner and the nsObjectFrame. Can you explain?

@@ +2030,5 @@
> +    NS_ASSERTION(pluginHost, "Without a pluginHost, how can we have an instance to destroy?");
> +    static_cast<nsPluginHost*>(pluginHost.get())->StopPluginInstance(inst);
> +    
> +    // the frame is going away along with its widget so tell the
> +    // window to forget its widget too

Fix comment

@@ +2193,5 @@
> +  if (aRecursionDepth == mDestroyRecursionDepth) {
> +    // If we don't have a parent node then destroy our plugin instance.
> +    nsCOMPtr<nsIContent> thisContent = do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
> +    if (!thisContent->GetParent()) {
> +      StopPluginInstance();

I'm not really sure what the destruction scheme here is. Can you document how we're using AfterProcessNextEvent, and why? Is there a reason you can't just use a regular runnable event?

@@ +2198,5 @@
> +    }
> +
> +    nsCOMPtr<nsIThreadInternal> thread = do_QueryInterface(aThread);
> +    NS_ASSERTION(thread, "This must never fail!");
> +    if (NS_FAILED(thread->RemoveObserver(this))) {

Could this be releasing the last reference to "this" and causing destruction of "this"?

::: content/base/src/nsObjectLoadingContent.h
@@ +145,5 @@
> +  
> +    nsresult InstantiatePluginInstance(nsIChannel* aChannel,
> +                                       nsIStreamListener** aStreamListener);
> +  
> +    nsresult InstantiatePluginInstance(const char* aMimeType, nsIURI* aURI);

Document that these flush layout.

::: dom/base/nsDOMClassInfo.cpp
@@ -9599,5 @@
>  
> -  // If it's not safe to run script we'll only return the instance if it
> -  // exists.
> -  if (!nsContentUtils::IsSafeToRunScript()) {
> -    return objlc->GetPluginInstance(_result);

Why is it OK to get rid of this check?

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +2910,5 @@
> +        nsCOMPtr<nsIPresShell> shell = doc->GetShell();
> +        if (shell) {
> +          nsIViewManager* vm = shell->GetViewManager();
> +          if (vm) {
> +            vm->GetRootWidget(getter_AddRefs(parentWidget));

Here I think we should share code with LayerManagerForDocumentInternal in nsContentUtils. That code goes to great lengths to find a widget and then gets a layer manager from it. We should refactor that code into a helper function to get a widget, and call that helper here. That will let you find a widget even if you're in a display:none IFRAME (where doc->GetShell() will return null). You probably should write a test for instantiating a plugin in a display:none IFRAME.

@@ +3273,5 @@
> +      }
> +    }
> +
> +    // Make sure the old frame isn't holding a reference to us.
> +    mObjectFrame->SetInstanceOwner(nsnull);

Do we need to SetWidget(null) on mObjectFrame?

Under what circumstances would we call SetObjectFrame with aFrame non-null and mObjectFrame non-null?

::: dom/plugins/base/nsPluginInstanceOwner.h
@@ +201,5 @@
>  #endif // XP_MACOSX
>    void CallSetWindow();
> +
> +  void SetObjectFrame(nsObjectFrame *aFrame);
> +  nsObjectFrame* GetObjectFrame();

Just call these SetFrame/GetFrame. "Object" isn't the right word, we should eventually rename nsObjectFrame to nsPluginFrame.

::: layout/generic/nsObjectFrame.cpp
@@ +440,5 @@
> +    // size, but just in case we haven't been reflowed or something, set
> +    // the size explicitly.
> +    nsAutoTArray<nsIWidget::Configuration,1> configuration;
> +    GetEmptyClipConfiguration(&configuration);
> +    if (configuration.Length() > 0) {

Just assert that the length is > 0.

::: widget/src/cocoa/nsChildView.mm
@@ +722,5 @@
> +
> +  mParentWidget = aNewParent;
> +
> +  if (mParentWidget) {
> +    mParentWidget->AddChild(this);

I think you should add documentation to nsIWidget::SetParent to indicate that null parents are allowed.

It looks like in gtk2, nsWindow::SetParent bails if mParent is currently null, which surely you need to fix. Maybe other problems in that method too, I stopped looking :-).

We'll definitely want to have tests that exercise SetParent to and from null.

The widget changes can and should go in their own patch.

::: widget/src/xpwidgets/nsBaseWidget.cpp
@@ +290,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsBaseWidget::UpdateEventCallback(EVENT_CALLBACK aEventFunction,
> +                                  nsDeviceContext *aContext)

Why not just SetEventCallback?
Comment 147 Josh Aas 2011-08-23 06:39:24 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #146)

> ::: content/base/src/nsObjectLoadingContent.cpp
> @@ +479,5 @@
> > +  if (!mInstanceOwner) {
> > +    return NS_ERROR_OUT_OF_MEMORY;
> > +  }
> > +
> > +  nsIObjectFrame* frame = GetExistingFrame(eFlushLayout);
> 
> So, this only flushes layout if there isn't already an existing frame. Is
> that really what you want? Or should we do a proper layout flush here to
> ensure that the size is more likely to be correct?

iirc I left it this way in order to minimize flushes. If the frame situation changes in the future we can recover from that. I didn't see any issues during testing. If you think it is worth flushing every time in order to minimize frame/size transitions I will try it.

objectFrame->FixupWindow(objectFrame->GetContentRectRelativeToSelf().Size());
> > +    if (weakFrame.IsAlive()) {
> > +      // Ensure we redraw when a plugin instance is instantiated
> > +      objectFrame->Invalidate(objectFrame->GetContentRectRelativeToSelf());
> > +    }
> 
> Can FixupWindow really destroy objectFrame? That seems surprising. If so,
> how? And what should happen in that case?

I assume it can because it can result in us calling into the plugin (NPP_SetWindow). If the frame dies here we just do nothing (skip the invalidate) but go ahead and instantiate, we'll deal with the new frame when it comes along.

> > +  if (aRecursionDepth == mDestroyRecursionDepth) {
> > +    // If we don't have a parent node then destroy our plugin instance.
> > +    nsCOMPtr<nsIContent> thisContent = do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
> > +    if (!thisContent->GetParent()) {
> > +      StopPluginInstance();
> 
> I'm not really sure what the destruction scheme here is. Can you document
> how we're using AfterProcessNextEvent, and why? Is there a reason you can't
> just use a regular runnable event?

When you suggest using a regular runnable event, are you suggesting that I have the event stop the plugin if the content doesn't have a parent? That might work better than my current scheme, which is a little ugly.
Comment 148 Josh Aas 2011-08-23 07:49:37 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #146)
> @@ +662,4 @@
> >    // 2) Our type hint is a type that we support with a plugin.
> > +  if (!mContentType.IsEmpty() &&
> > +      ((channelType.EqualsASCII(APPLICATION_OCTET_STREAM) &&
> > +        GetTypeOfContent(mContentType) != eType_Document) ||
> 
> Why have you changed the logic here?

There was a reason but it doesn't matter for this patch. I reverted the code and will deal with it in another bug.

> ::: dom/base/nsDOMClassInfo.cpp
> @@ -9599,5 @@
> >  
> > -  // If it's not safe to run script we'll only return the instance if it
> > -  // exists.
> > -  if (!nsContentUtils::IsSafeToRunScript()) {
> > -    return objlc->GetPluginInstance(_result);
> 
> Why is it OK to get rid of this check?

We always only get an existing object, we're always taking the safer route (no instantiation from here). If it was possible to instantiate here it would have been done already.
Comment 149 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 18:09:16 PDT
(In reply to Josh Aas (Mozilla Corporation) from comment #147)
> iirc I left it this way in order to minimize flushes. If the frame situation
> changes in the future we can recover from that. I didn't see any issues
> during testing. If you think it is worth flushing every time in order to
> minimize frame/size transitions I will try it.

I think you should at least run Tp4 with the current patch and check if there are any plugin resizes. If there are, see if always flushing layout here fixes them.

> objectFrame->FixupWindow(objectFrame->GetContentRectRelativeToSelf().Size());
> > > +    if (weakFrame.IsAlive()) {
> > > +      // Ensure we redraw when a plugin instance is instantiated
> > > +      objectFrame->Invalidate(objectFrame->GetContentRectRelativeToSelf());
> > > +    }
> > 
> > Can FixupWindow really destroy objectFrame? That seems surprising. If so,
> > how? And what should happen in that case?
> 
> I assume it can because it can result in us calling into the plugin
> (NPP_SetWindow). If the frame dies here we just do nothing (skip the
> invalidate) but go ahead and instantiate, we'll deal with the new frame when
> it comes along.

OK.

> > > +  if (aRecursionDepth == mDestroyRecursionDepth) {
> > > +    // If we don't have a parent node then destroy our plugin instance.
> > > +    nsCOMPtr<nsIContent> thisContent = do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
> > > +    if (!thisContent->GetParent()) {
> > > +      StopPluginInstance();
> > 
> > I'm not really sure what the destruction scheme here is. Can you document
> > how we're using AfterProcessNextEvent, and why? Is there a reason you can't
> > just use a regular runnable event?
> 
> When you suggest using a regular runnable event, are you suggesting that I
> have the event stop the plugin if the content doesn't have a parent? That
> might work better than my current scheme, which is a little ugly.

I'm not suggesting anything because I'm not sure what you're actually trying to do :-).
Comment 150 Josh Aas 2011-08-23 18:59:34 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #149)

> > > I'm not really sure what the destruction scheme here is. Can you document
> > > how we're using AfterProcessNextEvent, and why? Is there a reason you can't
> > > just use a regular runnable event?
> > 
> > When you suggest using a regular runnable event, are you suggesting that I
> > have the event stop the plugin if the content doesn't have a parent? That
> > might work better than my current scheme, which is a little ugly.
> 
> I'm not suggesting anything because I'm not sure what you're actually trying
> to do :-).

This code is limiting the scope in which a plugin will continue to run once its content is disconnected from the DOM. If we get back to the event loop and it still hasn't been re-parented then we stop it. So you can move the plugin content around the DOM without stopping it, but you can't leave it disconnected indefinitely without stopping it. I meant to look at what other browsers do but I forgot and this works pretty well. I'll poke around though.
Comment 151 Josh Aas 2011-08-23 19:17:09 PDT
Created attachment 555298 [details] [diff] [review]
fix v1.1

Includes fixes for some of what roc already pointed out, will do more tomorrow.
Comment 152 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 19:34:13 PDT
(In reply to Josh Aas (Mozilla Corporation) from comment #150)
> This code is limiting the scope in which a plugin will continue to run once
> its content is disconnected from the DOM. If we get back to the event loop
> and it still hasn't been re-parented then we stop it. So you can move the
> plugin content around the DOM without stopping it, but you can't leave it
> disconnected indefinitely without stopping it. I meant to look at what other
> browsers do but I forgot and this works pretty well. I'll poke around though.

OK, now I see. That needs to be documented, and even specced somewhere.

Either your current approach (stop the plugin if it's still disconnected when the current task finishes) or the approach of dispatching a new task to stop the plugin if it's still disconnected would work, it seems to me. The latter would be simpler to implement, so I'd go with that all other things being equal. I think this definitely needs testing in other browsers though, if there is any interoperability there we should make sure we follow it.
Comment 153 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 20:43:28 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #149)
> (In reply to Josh Aas (Mozilla Corporation) from comment #147)
> > iirc I left it this way in order to minimize flushes. If the frame situation
> > changes in the future we can recover from that. I didn't see any issues
> > during testing. If you think it is worth flushing every time in order to
> > minimize frame/size transitions I will try it.
> 
> I think you should at least run Tp4 with the current patch and check if
> there are any plugin resizes. If there are, see if always flushing layout
> here fixes them.

Actually, assuming most plugin instances have "width" and "height" attributes that match the width and height they will actually have, which seems to be true in my non-scientific sampling, perhaps we should just not flush at all here, and whenever the plugin either has no frame or the frame has not had its first reflow yet, set the width and height of the plugin to the values given by the attributes (or the default 300/150 otherwise).
Comment 154 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-23 20:44:31 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #153)
> set the width and height of the plugin to
> the values given by the attributes (or the default 300/150 otherwise)

Perhaps modified by the zoom level of the document's presentation (if there is one).
Comment 155 Jonas Sicking (:sicking) 2011-08-24 08:35:52 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #152)
> Either your current approach (stop the plugin if it's still disconnected
> when the current task finishes) or the approach of dispatching a new task to
> stop the plugin if it's still disconnected would work, it seems to me. The
> latter would be simpler to implement, so I'd go with that all other things
> being equal. I think this definitely needs testing in other browsers though,
> if there is any interoperability there we should make sure we follow it.

The downside with stopping it in a separate task is that it's somewhat "racy". I.e. if a setTimeout or requestAnimationFrame callback fires, it may or may not happen before the event to terminate the plugin.
Comment 156 Josh Aas 2011-08-24 10:23:07 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #146)

> @@ +3273,5 @@
> > +      }
> > +    }
> > +
> > +    // Make sure the old frame isn't holding a reference to us.
> > +    mObjectFrame->SetInstanceOwner(nsnull);
> 
> Do we need to SetWidget(null) on mObjectFrame?

Object frames can't have a widget unless they have an instance owner under this scheme. nsObjectFrame::SetInstanceOwner(nsnull) cuts the widget loose here.

> Under what circumstances would we call SetObjectFrame with aFrame non-null
> and mObjectFrame non-null?

I don't think that can happen right now but the function is coded defensively re: that situation. There's no reason not to allow it, it just isn't done right now. All SetFrame calls with a non-null frame happen on initialization or after a DisconnectFrame() call.
Comment 157 Josh Aas 2011-08-24 17:24:14 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #146)

> I think you should add documentation to nsIWidget::SetParent to indicate
> that null parents are allowed.
> 
> It looks like in gtk2, nsWindow::SetParent bails if mParent is currently
> null, which surely you need to fix. Maybe other problems in that method too,
> I stopped looking :-).

Nice catch! I missed that because my re-parenting tests on Linux only involved windowless plugins.
Comment 158 Josh Aas 2011-08-24 19:53:18 PDT
Created attachment 555626 [details] [diff] [review]
fix v1.2

More fixes, still have a couple of comments to address.
Comment 159 Josh Aas 2011-08-25 14:07:16 PDT
Created attachment 555832 [details] [diff] [review]
widget fix v1.3
Comment 160 Josh Aas 2011-08-25 14:08:28 PDT
Created attachment 555834 [details] [diff] [review]
other fix v1.3
Comment 161 Josh Aas 2011-08-25 14:15:17 PDT
There is now a windowed version of the re-parenting test for Linux and Windows that tests the widget->SetParent(NULL) case. If you want and have a suggestion for a regular widget test I'll do it.
Comment 162 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-25 15:42:55 PDT
Comment on attachment 555832 [details] [diff] [review]
widget fix v1.3

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

Looks good to me. Passing review to Karl for the GTK code.
Comment 163 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-25 15:51:53 PDT
Comment on attachment 555834 [details] [diff] [review]
other fix v1.3

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

::: content/base/public/nsContentUtils.h
@@ +1662,5 @@
>                                           const nsAString& aClasses,
>                                           nsIDOMNodeList** aReturn);
>  
>    /**
> +   * Returns the widget for a document.

This needs more documentation. Say something like
"Returns the widget for this document if there is one. Looks at all ancestor documents to try to find a widget, so for example this can still find a widget for documents in display:none frames that have no presentation."

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1493,5 @@
> +    mDestroyRecursionDepth = depth - 1;
> +    
> +    thread->AddObserver(this);
> +
> +    mObservingThread = PR_TRUE;

I think instead of this we could just use nsIAppShell::RunInStableState. It means that if you remove a plugin and then spin a nested event loop and then put it back, we'll have destroyed the plugin instance, but really, does it matter? I can't imagine people intentionally doing that.
Comment 164 Karl Tomlinson (ni?:karlt) 2011-08-25 18:24:45 PDT
Comment on attachment 555832 [details] [diff] [review]
widget fix v1.3

>+    NS_IMETHOD SetEventCallback(EVENT_CALLBACK aEventFunction,
>+                                nsDeviceContext *aContext) = 0;

>+{
>+  mEventCallback = aEventFunction;
>+
>+  if (aContext) {
>+    NS_IF_RELEASE(mContext);
>+    mContext = aContext;
>+    NS_ADDREF(mContext);
>+  }
>+
>+  return NS_OK;
>+}

Can you document the behaviour re keeping the old device context when aContext
is null, please?

> NS_IMETHODIMP
> nsWindow::SetParent(nsIWidget *aNewParent)
> {
>-    if (mContainer || !mGdkWindow || !mParent) {
>-        NS_NOTREACHED("nsWindow::SetParent - reparenting a non-child window");
>+    if (mContainer || !mGdkWindow) {
>+        NS_NOTREACHED("nsWindow::SetParent called illegally");
>         return NS_ERROR_NOT_IMPLEMENTED;

Need to CheckDestroyInvisibleContainer()
if oldContainer == gInvisibleContainer
(unless newContainer is also gInvisibleContainer).

After SetWidgetForHierarchy in ReparentNativeWidgetInternal would be a good
place.
Comment 165 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-25 21:29:45 PDT
*** Bug 647943 has been marked as a duplicate of this bug. ***
Comment 166 Josh Aas 2011-08-26 22:43:40 PDT
Created attachment 556221 [details] [diff] [review]
widget fix v1.4
Comment 167 Josh Aas 2011-08-28 10:40:55 PDT
Created attachment 556404 [details] [diff] [review]
other fix v1.4
Comment 168 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-28 15:38:47 PDT
Comment on attachment 556404 [details] [diff] [review]
other fix v1.4

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

Please add a test to ensure that when you remove a plugin from the document it is destroyed before the next event. I would do a setTimeout(0, checkPluginAlreadyDestroyed) followed by removing the plugin from the document.

Also, please add a test for the plugin being removed, added, then removed again. Two parent-check events will run and we need to make sure we don't crash.

Also, add a test that removes a subtree of content from the document which contains a plugin, and checks that the plugin is destroyed. I think you actually have bug here at the moment: instead of testing for null parent, you need to check whether the plugin is in its document.
Comment 169 Karl Tomlinson (ni?:karlt) 2011-08-28 16:29:02 PDT
Comment on attachment 556221 [details] [diff] [review]
widget fix v1.4

>         if (aNewContainer != aOldContainer) {
>             NS_ABORT_IF_FALSE(!GDK_WINDOW_OBJECT(aNewParentWindow)->destroyed,
>                               "destroyed GdkWindow with widget");
>             SetWidgetForHierarchy(mGdkWindow, aOldContainer, aNewContainer);
>+
>+            if (aOldContainer == gInvisibleContainer &&
>+                aNewContainer != gInvisibleContainer) {

Please remove the "aNewContainer != gInvisibleContainer" part of the test as here aNewContainer != aOldContainer (and aOldContainer == gInvisibleContainer).
Comment 170 Christian :Biesinger (don't email me, ping me on IRC) 2011-08-28 17:53:38 PDT
Comment on attachment 556221 [details] [diff] [review]
widget fix v1.4

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

::: widget/public/nsIWidget.h
@@ +363,5 @@
>                  nsWidgetInitData *aInitData = nsnull,
>                  PRBool           aForceUseIWidgetParent = PR_FALSE) = 0;
>  
>      /**
> +     * Set the event callback for a widget. If a device conext is not

typo: conext -> context
Comment 171 Christian :Biesinger (don't email me, ping me on IRC) 2011-08-28 20:11:43 PDT
Comment on attachment 556404 [details] [diff] [review]
other fix v1.4

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

Thanks for doing this! Some comments below.

::: content/base/src/nsObjectLoadingContent.cpp
@@ -159,5 @@
> -      nsCAutoString spec;
> -      if (mURI) {
> -        mURI->GetSpec(spec);
> -      }
> -      LOG(("OBJLC [%p]: Handling Instantiate event: Type=<%s> URI=%p<%s>\n",

Maybe keep this LOG statement?

@@ +516,5 @@
> +  nsresult rv = mInstanceOwner->Init(objectFrame, thisContent);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID, &rv));
> +  nsPluginHost *pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());

Seems nicer to move InstantiatePluginForChannel to the IDL instead of this cast

@@ +562,5 @@
> +  if (!mInstanceOwner)
> +    return NS_ERROR_OUT_OF_MEMORY;
> +
> +  nsIObjectFrame* frame = GetExistingFrame(eFlushLayout);
> +  nsObjectFrame* objectFrame = static_cast<nsObjectFrame*>(frame);

this is the second time I see in this patch that does the cast, maybe GetExistingFrame should do the cast instead

@@ +569,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // get the nsIPluginHost service
> +  nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID, &rv));
> +  nsPluginHost* pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());

Can you also add the method you need to the interface?

@@ +587,5 @@
> +  if (pDoc) {
> +    pDoc->GetWillHandleInstantiation(&fullPageMode);
> +  }
> +
> +  if (fullPageMode) {  /* full-page mode */

that comment seems kinda useless

@@ +606,5 @@
> +
> +  // Set up scripting interfaces.
> +  NotifyContentObjectWrapper();
> +
> +  // This is necessary here for some reason.

(I hate plugin code so much)

@@ +616,5 @@
> +  GetPluginInstance(getter_AddRefs(pluginInstance));
> +  if (pluginInstance) {
> +    nsCOMPtr<nsIPluginTag> pluginTag;
> +    nsCOMPtr<nsIPluginHost> host(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID));
> +    static_cast<nsPluginHost*>(host.get())->

No need to getService again, you still have pluginHost above

@@ -574,5 @@
>    // Now find out what type the content is
>    // UnloadContent will set our type to null; need to be sure to only set it to
>    // the real value on success
>    ObjectType newType = GetTypeOfContent(mContentType);
> -  LOG(("OBJLC [%p]: OnStartRequest: Content Type=<%s> Old type=%u New Type=%u\n",

Any particular reason you removed this and other logs? I found them fairly useful when writing and debugging this code.

@@ -758,5 @@
> -      nsIObjectFrame* frame = GetExistingFrame(eFlushContent);
> -      if (frame) {
> -        // We have to notify the wrapper here instead of right after
> -        // Instantiate because the plugin only gets instantiated by
> -        // OnStartRequest, not by Instantiate.

Oh god the memories

@@ +1505,5 @@
>      // have already loaded the content.
>      mURI = nsnull;
>    }
> +
> +  nsCOMPtr<nsIContent> thisContent = do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));

I feel this code block deserves a comment. I understand it now, in part because we discussed it on irc, but it is not necessarily obvious.

@@ +1947,5 @@
> +nsObjectLoadingContent::StartPluginInstance()
> +{
> +  // OK to have an instance already.
> +  if (mInstanceOwner) {
> +    return NS_OK;

Hm... is this correct? Don't you have to still call Instantiate in this case? Consider an <object data="foo.swf"> getting a .setAttribute("data", "bar.mov") call.

@@ +1973,5 @@
> +
> +  return rv;
> +}
> +
> +class nsStopPluginRunnable : public nsRunnable, public nsITimerCallback

This should probably be moved up in the file to where the other events are declared

@@ +2033,5 @@
> +    
> +    nsPluginNativeWindow *window = (nsPluginNativeWindow *)win;
> +    if (window) {
> +      nsRefPtr<nsNPAPIPluginInstance> nullinst;
> +      window->CallSetWindow(nullinst);

nullinstant->nsnull? Or does that not work?

@@ +2145,5 @@
> +  }
> +  
> +  // The plugin may have set up new interfaces; we need to mess with our JS
> +  // wrapper.  Note that we DO NOT want to call this if there is no plugin
> +  // instance!  That would just reenter Instantiate(), trying to create

I believe the Instantiate part of the comment is outdated? At least I don't see right know where NotifyContentObjectWrapper would call Instantiate.
Comment 172 Josh Aas 2011-08-28 21:19:17 PDT
Created attachment 556468 [details] [diff] [review]
widget fix v1.5
Comment 173 Josh Aas 2011-08-28 21:20:34 PDT
Created attachment 556469 [details] [diff] [review]
other fix v1.5
Comment 174 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-28 21:28:53 PDT
Comment on attachment 556469 [details] [diff] [review]
other fix v1.5

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +157,4 @@
>  
> +// Checks to see if the content for a plugin instance has a parent.
> +// The plugin instance is stopped if there is no parent.
> +class nsParentCheckEvent : public nsRunnable {

Probably should call this InDocCheckEvent now. (No need for "ns" prefix.)
Comment 175 Josh Aas 2011-08-28 22:30:24 PDT
Created attachment 556476 [details] [diff] [review]
other fix v1.6
Comment 176 Christian :Biesinger (don't email me, ping me on IRC) 2011-08-28 22:40:45 PDT
There's still a fair amount of log statements you removed, any specific reason?

Also, did you want to file a new bug about this comment:
"Hm... is this correct? Don't you have to still call Instantiate in this case? Consider an <object data="foo.swf"> getting a .setAttribute("data", "bar.mov") call."
Comment 177 Josh Aas 2011-08-28 23:18:57 PDT
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #176)
> There's still a fair amount of log statements you removed, any specific
> reason?

I just didn't find them useful, though I was admittedly overzealous at first and removed too many.

> Also, did you want to file a new bug about this comment:
> "Hm... is this correct? Don't you have to still call Instantiate in this
> case? Consider an <object data="foo.swf"> getting a .setAttribute("data",
> "bar.mov") call."

Yeah, I'm planning to file a bug about that.
Comment 179 Josh Heidenreich 2011-08-29 16:45:43 PDT
Josh Aas,

Thank You!!!
Comment 180 pal-moz 2011-08-29 16:53:02 PDT
some Flash content is no longer loaded after this checkin.

http://forums.mozillazine.org/viewtopic.php?p=11191871#p11191871
Comment 181 Jo Hermans 2011-08-29 17:25:40 PDT
Another example : <http://www.spelletjes.nl/spel/spider-soli.html>. There's supposed to be a Solitaire game loaded on this site, but the progress bar stays at 0%. The 2 adds above and below (which are also Flash) work ok. Note that both the adds and the progressbar go away after a while (or they don't redisplay anymore), that's normal.

I don't have Flashblock or AdBlock installed. I know for sure that the earlier update this morning (which didn't contain bug 90268), allowed me to play this game. Now I can't anymore.

I'll wait a while before I file a bug, it's 2AM now. I'll try to debug it better when I'm awake. And if I can convince my boss that playing Solitaire is actually debugging :-)
Comment 182 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-29 21:28:38 PDT
I think we should probably back this out now (while it's still easy to do so) and fix these regressions before relanding.
Comment 184 Josh Aas 2011-08-30 19:34:05 PDT
Created attachment 557076 [details] [diff] [review]
other fix v1.7

Updated to current trunk, no further fixes.
Comment 185 AL 2011-09-03 23:11:07 PDT
1. Have the specific regressions been pinpointed, and if so, what are they?
2. When you say you updated to current trunk, do you mean that you have a parallel build that you are maintaining with all your bug 90268 patches? If so, what is the merge process?
Comment 186 Justin Lebar (not reading bugmail) 2011-09-04 08:28:24 PDT
(In reply to AL from comment #185)
> 1. Have the specific regressions been pinpointed, and if so, what are they?

See "Depends on:" at the top of the bug.

> 2. When you say you updated to current trunk, do you mean that you have a
> parallel build that you are maintaining with all your bug 90268 patches? If
> so, what is the merge process?

He means that his patch, which is attached to the comment, cleanly applies to the current trunk.  When he's ready to check in, he'll apply the patch and push the change; there is no merge process.
Comment 187 Josh Aas 2011-09-06 06:10:58 PDT
Created attachment 558456 [details] [diff] [review]
other fix v1.8

Mainly improvements to code paths leading to NPP_SetWindow.
Comment 188 AL 2011-09-06 11:36:30 PDT
1. What kinds of improvements were made? Are you referring to better code organizations, or is there actually any optimization occurring at this stage?
2. How did "improving code paths" lead to a resolution of bug 674224, or was that a side effect?

Thanks!
Comment 189 j.j. 2011-09-06 12:02:25 PDT
AL, Bugzilla isn't a message board. Please read
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
(especially point 1.1)
Comment 190 Josh Aas 2011-09-06 12:47:51 PDT
(In reply to AL from comment #188)
> 1. What kinds of improvements were made? Are you referring to better code
> organizations, or is there actually any optimization occurring at this stage?

Code reorganization to make SetWindow calling schemes easier to read and maintain.

> 2. How did "improving code paths" lead to a resolution of bug 674224, or was
> that a side effect?

I don't know what exactly fixed that but I used to be able to reproduce it and now I can't.
Comment 191 Chris Pearce (:cpearce) 2011-09-18 15:41:32 PDT
I've discovered a regression with these patches while developing my patches in bug 684618 on top of the patches here. I'm not sure if you're aware of it... If you add a plugin to a document, remove it from the document, then re-add it to the document, the plugin does not start rendering again. This works fine on trunk. Testcase with STR here: http://pearce.org.nz/full-screen/90268-bug.html
Comment 192 Josh Aas 2011-09-23 14:06:41 PDT
Created attachment 562159 [details] [diff] [review]
widget fix v1.6

Updated to current trunk.
Comment 193 Josh Aas 2011-09-23 14:10:07 PDT
Created attachment 562160 [details] [diff] [review]
other fix v1.9

Updated to current trunk with a few minor changes.
Comment 194 Josh Aas 2011-09-25 19:19:10 PDT
Created attachment 562343 [details] [diff] [review]
other fix v2.0

Nice catch in comment 191, Chris. Thanks. This patch has a fix with a test.
Comment 195 Josh Aas 2011-09-27 23:29:05 PDT
Created attachment 562982 [details] [diff] [review]
other fix v2.1

This includes a number of fixes, including a fix for Google Earth. Problem is, now Netflix doesn't work on Mac OS X.
Comment 196 Josh Aas 2011-10-05 14:12:40 PDT
Created attachment 565004 [details] [diff] [review]
widget fix v1.7

Updated to current trunk.
Comment 197 Josh Aas 2011-10-05 14:18:36 PDT
Created attachment 565011 [details] [diff] [review]
other fix v2.2

Updated to current trunk. Still has at least two issues:

1) "test_zero_opacity.html" fails on Linux due to an assertion.
2) Netflix doesn't work on Mac OS X.
Comment 198 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-07 01:40:53 PDT
*** Bug 647943 has been marked as a duplicate of this bug. ***
Comment 199 Josh Aas 2011-10-09 21:30:07 PDT
Created attachment 565860 [details] [diff] [review]
other fix v2.3

Fixes a number of test failures.
Comment 200 Boris Zbarsky [:bz] 2011-10-11 11:34:33 PDT
*** Bug 690525 has been marked as a duplicate of this bug. ***
Comment 201 Martin Best (:mbest) 2011-10-19 06:58:29 PDT
How is your feeling on this bug being resolved before Firefox 10 lands in Aurora.  545812 is dependent on this landing and we are trying to get a sense of the risk.
Comment 202 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-10-19 07:08:28 PDT
This might be a silly question, but why does Bug 545812 depend on this?
Comment 203 Martin Best (:mbest) 2011-10-19 07:54:53 PDT
Sorry, my bad, should have been 684618.
Comment 204 Martin Best (:mbest) 2011-10-19 07:57:57 PDT
See comment 191 for the test case, looks resolved but we are trying to figure out if it will land.
Comment 205 Josh Aas 2011-10-19 22:13:10 PDT
Created attachment 568302 [details] [diff] [review]
widget fix v1.8

Updated to current m-c.
Comment 206 Josh Aas 2011-10-19 22:14:10 PDT
Created attachment 568303 [details] [diff] [review]
other fix v2.4

Updated to current m-c.
Comment 207 Josh Aas 2011-10-19 22:15:42 PDT
(In reply to mbest@mozilla.com from comment #201)
> How is your feeling on this bug being resolved before Firefox 10 lands in
> Aurora.  545812 is dependent on this landing and we are trying to get a
> sense of the risk.

I have a couple more bugs to fix. I'm working on them now, hard to say if they'll take six hours or two weeks. My feeling on the FF10 deadline is that we either land this soon (within a week) or we wait for FF11. I don't want to land it too close to the deadline. I'll post patches here as soon as I have further fixes.
Comment 208 Josh Aas 2011-10-20 09:35:22 PDT
Created attachment 568420 [details] [diff] [review]
other fix v2.5

Updated to current m-c.
Comment 209 Mathieu Beausoleil 2011-10-20 21:45:17 PDT
Created attachment 568600 [details]
An other way to get the bug

Get the bug just by changing the css positioning of the container.
Comment 210 Josh Aas 2011-10-25 12:07:43 PDT
Created attachment 569455 [details] [diff] [review]
other fix v2.6
Comment 211 Josh Aas 2011-10-25 12:18:17 PDT
The only remaining test failure on Linux is "test_zero_opacity.html". It times out with the assertion "Widgets that we paint must all be display roots".
Comment 212 Karl Tomlinson (ni?:karlt) 2011-10-25 19:41:12 PDT
Comment on attachment 569455 [details] [diff] [review]
other fix v2.6

(In reply to Josh Aas (Mozilla Corporation) from comment #211)
> The only remaining test failure on Linux is "test_zero_opacity.html". It
> times out with the assertion "Widgets that we paint must all be display
> roots".

>@@ -1071,17 +1071,17 @@ nsPluginHost::InstantiateEmbeddedPlugin(
>   // but we had a problem with the entry point
>   if (rv == NS_ERROR_FAILURE)
>     return rv;
> 
>   if (instance) {
>     aOwner->CreateWidget();
> 
>     // If we've got a native window, the let the plugin know about it.
>-    aOwner->SetWindow();
>+    aOwner->CallSetWindow();

>-NS_IMETHODIMP nsPluginInstanceOwner::SetWindow()
>-{
>-  NS_ENSURE_TRUE(mObjectFrame, NS_ERROR_NULL_POINTER);
>-  return mObjectFrame->CallSetWindow(false);
>-}

These changes are causing nsPluginNativeWindowGtk2::CallSetWindow() to be
skipped and instead nsNPAPIPluginInstance::SetWindow() is getting called
directly.  This means that the browser's socket window is not created, and
instead the intended parent window of the socket is passed to the plugin.

data:text/html,<embed type="application/x-test" wmode="window">
is a suitable test case.
Comment 213 Josh Aas 2011-10-26 12:56:18 PDT
Thanks Karl! Very helpful, I have a fix which will be in my next patch.
Comment 214 Josh Aas 2011-10-26 14:25:34 PDT
Created attachment 569797 [details] [diff] [review]
widget fix v1.9
Comment 215 Josh Aas 2011-10-26 14:28:42 PDT
Created attachment 569798 [details] [diff] [review]
plugin fix v2.7

All tests pass locally on Windows, Linux, and Mac OS X with this patch. Netflix is still broken on Windows and Mac OS X (didn't try on Linux).
Comment 216 Josh Aas 2011-10-26 18:19:40 PDT
It seems like my patch makes intermittent orange bug 565953 happen more often. It's still intermittent but > 50% on tbox runs with my latest patch. Looks like we'll have to resolve that in addition to fixing the Netflix issue before we can take these patches.

ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/test_pluginstream_err.html | test frame has unexpected content - got NPP_StreamAsFile calledpass, expected pass
Comment 217 Karl Tomlinson (ni?:karlt) 2011-10-27 20:51:49 PDT
Created attachment 570172 [details]
testcase showing reset on ancestor style change

Updating attachment 410850 [details] to use a url that is not 404.
Comment 218 j.j. 2011-10-31 08:13:14 PDT
Changing Target Milestone to 11 based on comment 207
Comment 219 Josh Aas 2011-11-03 09:21:59 PDT
Created attachment 571673 [details] [diff] [review]
plugin fix v2.8

Updated to current m-c. I'll be removing initialization for display:none plugins next, per the discussion in bug 697651.
Comment 220 Josh Aas 2011-11-03 13:23:54 PDT
Created attachment 571752 [details] [diff] [review]
plugin fix v2.9

Removes support for instantiating display:none instances. Also fixes a crash I've been seeing once in a while and finally tracked down.
Comment 221 Josh Aas 2011-11-03 15:54:03 PDT
The latest patch introduces some new test failures, shouldn't be hard to fix though. Patch coming up soon.
Comment 222 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-10 23:27:08 PST
I've been running with these patches for a while now and generally things have been looking good here. But I did run into a crash today that I was able to reproduce and track down, and while I think I'm on a version behind the current version it looks at a quick glance as if the crash I was seeing is still in the most recent patch.

The problem I ran into was that we ended up creating two nsPluginInstanceOwner's for the same content and frame, the reason for that is shown in this stack:

#0  nsPluginInstanceOwner::Init (this=0x7fffada0f300, aFrame=0x7fffb64f82d8, 
    aContent=0x7fffb669da80)
    at /home/jst/fast/work/tip/mozilla/dom/plugins/base/nsPluginInstanceOwner.cpp:3106
#1  0x00007ffff27e64a1 in nsObjectLoadingContent::InstantiatePluginInstance (
    this=0x7fffb669daf8, aMimeType=
    0x7fffbdc51df8 "application/x-shockwave-flash", aURI=0x7fffcda9d980)
    at /home/jst/fast/work/tip/mozilla/content/base/src/nsObjectLoadingContent.cpp:678
#2  0x00007ffff27eb43a in nsObjectLoadingContent::SyncStartPluginInstance (
    this=0x7fffb669daf8)
    at /home/jst/fast/work/tip/mozilla/content/base/src/nsObjectLoadingContent.cpp:2071
#3  0x00007ffff2bb169f in nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe (
    wrapper=0x7fffd08b8900, obj=0x7fffd0fb9710, _result=0x7ffffffef810)
    at /home/jst/fast/work/tip/mozilla/dom/base/nsDOMClassInfo.cpp:9381
#4  0x00007ffff2bb2734 in nsHTMLPluginObjElementSH::NewResolve (this=
    0x7fffb28e86d0, wrapper=0x7fffd08b8900, cx=0x7fffbdc1e400, obj=
    0x7fffd0fb9710, id=..., flags=5, objp=0x7ffffffef9e8, _retval=
    0x7ffffffef9f7)
    at /home/jst/fast/work/tip/mozilla/dom/base/nsDOMClassInfo.cpp:9702
#5  0x00007ffff3110fd4 in XPC_WN_Helper_NewResolve (cx=0x7fffbdc1e400, obj=
    0x7fffd0fb9710, id=..., flags=5, objp=0x7ffffffefad8)
    at /home/jst/fast/work/tip/mozilla/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1118
#6  0x00007ffff3e3576f in CallResolveOp (cx=0x7fffbdc1e400, start=
    0x7fffd0fb9710, obj=0x7fffd0fb9710, id=..., flags=5, objp=0x7ffffffefc08, 
    propp=0x7ffffffefc00, recursedp=0x7ffffffefb87)
    at /home/jst/fast/work/tip/mozilla/js/src/jsobj.cpp:5404
#7  0x00007ffff3e359e6 in LookupPropertyWithFlagsInline (cx=0x7fffbdc1e400, 
    obj=0x7fffd0fb9710, id=..., flags=65535, objp=0x7ffffffefc08, propp=
    0x7ffffffefc00) at /home/jst/fast/work/tip/mozilla/js/src/jsobj.cpp:5457
#8  0x00007ffff3e36aa0 in js_GetPropertyHelperInline (cx=0x7fffbdc1e400, obj=
    0x7fffd0fb9710, receiver=0x7fffd0fb9710, id=..., getHow=3, vp=
    0x7fffffff02b0) at /home/jst/fast/work/tip/mozilla/js/src/jsobj.cpp:5853
#9  0x00007ffff3e36f0d in js_GetPropertyHelper (cx=0x7fffbdc1e400, obj=
    0x7fffd0fb9710, id=..., getHow=3, vp=0x7fffffff02b0)
    at /home/jst/fast/work/tip/mozilla/js/src/jsobj.cpp:5946
#10 0x00007ffff3dfec0f in js::Interpret (cx=0x7fffbdc1e400, entryFrame=
    0x7fffdc0eb6b8, interpMode=js::JSINTERP_NORMAL)
    at /home/jst/fast/work/tip/mozilla/js/src/jsinterp.cpp:3487
#11 0x00007ffff3df1c24 in js::RunScript (cx=0x7fffbdc1e400, script=
    0x7fffd0fadd78, fp=0x7fffdc0eb6b8)
    at /home/jst/fast/work/tip/mozilla/js/src/jsinterp.cpp:584
#12 0x00007ffff3df1f61 in js::InvokeKernel (cx=0x7fffbdc1e400, args=..., 
    construct=js::NO_CONSTRUCT)
    at /home/jst/fast/work/tip/mozilla/js/src/jsinterp.cpp:647
#13 0x00007ffff3d5f72b in js::Invoke (cx=0x7fffbdc1e400, args=..., construct=
    js::NO_CONSTRUCT) at /home/jst/fast/work/tip/mozilla/js/src/jsinterp.h:148
#14 0x00007ffff3df214d in js::Invoke (cx=0x7fffbdc1e400, thisv=..., fval=..., 
    argc=0, argv=0x0, rval=0x7fffffff1400)
    at /home/jst/fast/work/tip/mozilla/js/src/jsinterp.cpp:679
#15 0x00007ffff3d3e2bd in JS_CallFunctionValue (cx=0x7fffbdc1e400, obj=
    0x7fffd0fb9710, fval=..., argc=0, argv=0x0, rval=0x7fffffff1400)
    at /home/jst/fast/work/tip/mozilla/js/src/jsapi.cpp:5200
#16 0x00007ffff2ac086d in nsXBLProtoImplAnonymousMethod::Execute (this=
    0x7fffb3020d60, aBoundElement=0x7fffb669da80)
    at /home/jst/fast/work/tip/mozilla/content/xbl/src/nsXBLProtoImplMethod.cpp:367
#17 0x00007ffff2aa9bdb in nsXBLPrototypeBinding::BindingAttached (this=
    0x7fffb5c31980, aBoundElement=0x7fffb669da80)
    at /home/jst/fast/work/tip/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp:522
#18 0x00007ffff2aa5953 in nsXBLBinding::ExecuteAttachedHandler (this=
    0x7fffb64df440)
    at /home/jst/fast/work/tip/mozilla/content/xbl/src/nsXBLBinding.cpp:949
#19 0x00007ffff2ad3a3f in nsBindingManager::ProcessAttachedQueue (this=
    0x7fffcee8bf90, aSkipSize=0)
    at /home/jst/fast/work/tip/mozilla/content/xbl/src/nsBindingManager.cpp:1049
#20 0x00007ffff244269f in PresShell::FlushPendingNotifications (this=
    0x7fffb91f2400, aType=Flush_Layout)
    at /home/jst/fast/work/tip/mozilla/layout/base/nsPresShell.cpp:4061
#21 0x00007ffff277bab9 in nsDocument::FlushPendingNotifications (this=
    0x7fffb8f62800, aType=Flush_Layout)
    at /home/jst/fast/work/tip/mozilla/content/base/src/nsDocument.cpp:6275
#22 0x00007ffff27eac84 in nsObjectLoadingContent::GetExistingFrame (this=
    0x7fffbda78c50, aFlushType=nsObjectLoadingContent::eFlushLayout)
    at /home/jst/fast/work/tip/mozilla/content/base/src/nsObjectLoadingContent.cpp:1916
#23 0x00007ffff27e63aa in nsObjectLoadingContent::InstantiatePluginInstance (
    this=0x7fffbda78c50, aMimeType=0x7ffff54aec40 "", aURI=0x7fffb8f052e0)
    at /home/jst/fast/work/tip/mozilla/content/base/src/nsObjectLoadingContent.cpp:658
#24 0x00007ffff27eb43a in nsObjectLoadingContent::SyncStartPluginInstance (
    this=0x7fffbda78c50)
    at /home/jst/fast/work/tip/mozilla/content/base/src/nsObjectLoadingContent.cpp:2071
#25 0x00007ffff2bb169f in nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe (
    wrapper=0x7fffd08b8120, obj=0x7fffd0fb95b0, _result=0x7fffffff19c0)
    at /home/jst/fast/work/tip/mozilla/dom/base/nsDOMClassInfo.cpp:9381
#26 0x00007ffff2bb1b63 in nsHTMLPluginObjElementSH::SetupProtoChain (wrapper=
    0x7fffd08b8120, cx=0x7fffbdc1e400, obj=0x7fffd0fb95b0)
    at /home/jst/fast/work/tip/mozilla/dom/base/nsDOMClassInfo.cpp:9448
#27 0x00007ffff2bba916 in nsPluginProtoChainInstallRunner::Run (this=
    0x7fffb28e8670)
    at /home/jst/fast/work/tip/mozilla/dom/base/nsDOMClassInfo.cpp:9415
#28 0x00007ffff27310f5 in nsContentUtils::RemoveScriptBlocker ()
    at /home/jst/fast/work/tip/mozilla/content/base/src/nsContentUtils.cpp:4427
#29 0x00007ffff244cb59 in PresShell::DidCauseReflow (this=0x7fffb91f2400)
    at /home/jst/fast/work/tip/mozilla/layout/base/nsPresShell.cpp:7175
#30 0x00007ffff2454456 in nsAutoCauseReflowNotifier::~nsAutoCauseReflowNotifier
    (this=0x7fffffff1bb0, __in_chrg=<value optimized out>)
    at /home/jst/fast/work/tip/mozilla/layout/base/nsPresShell.cpp:729
#31 0x00007ffff243c181 in PresShell::InitialReflow (this=0x7fffb91f2400, 
    aWidth=68340, aHeight=54180)
    at /home/jst/fast/work/tip/mozilla/layout/base/nsPresShell.cpp:1957
#32 0x00007ffff2720a3f in nsContentSink::StartLayout (this=0x7fffcee8cda0, 
    aIgnorePendingSheets=false)
    at /home/jst/fast/work/tip/mozilla/content/base/src/nsContentSink.cpp:1321
...

Specifically in nsObjectLoadingContent::InstantiatePluginInstance() at frame 23. There we're about to create a nsPluginInstanceOwner, and we null checked at the top of that function to make sure we didn't have one, but then we call GetExistingFrame(), which re-enters nsObjectLoadingContent::InstantiatePluginInstance() and creates a nsPluginInstanceOwner for the nsObjectLoadingContent, and then once we unwind to frame 23 we'll create another one. That leaves the first one kinda dangling, and later on we'll crash after the frame got destroyed since the first nsPluginInstanceOwner was never told the frame went away etc etc.

I'm not sure what the best way of fixing that in the new setup is. I'm right now running with a change to re-check mInstanceOwner after the call to GetExistingFrame() and bail early, but I'm by no means convinced that's the right fix here. And even with that we get asserts about LoadObject() re-entering as well.
Comment 223 Josh Aas 2011-11-14 08:25:13 PST
Created attachment 574308 [details] [diff] [review]
widget fix v2.0

Updated to current m-c.
Comment 224 Josh Aas 2011-11-14 08:29:41 PST
Created attachment 574310 [details] [diff] [review]
plugin fix 3.0

Made some progress over the weekend. This patch fixes Netflix, a crash, and some test failures. Two more test failures are the only known remaining issues.

Thanks for the report Johnny! This patch might fix your crash, I'll look more closely at it today though.
Comment 225 Josh Aas 2011-11-15 20:14:19 PST
Created attachment 574784 [details] [diff] [review]
plugin fix 3.1

Fixes test failures. The only reliably failing test remaining is "test_pluginstream_err.html" on Windows and Linux.
Comment 226 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-17 00:39:04 PST
Josh, looks like the crash I was seeing above still exists in the latest patch :(

I also found another crash with these patches, so far I've only been able to trigger it with flashblock installed. Start firefox, open a new window, load http://cdn-static.viddler.com/flash/as3/simple-publisher.swf?key=cb852767&ref=www.google.com in it, close the window (w/o even clicking to play the flash content) and I crash with the following signature:

#0  0x00007ffff242c7c2 in nsCOMPtr<nsIViewManager>::nsCOMPtr (this=
    0x7fffffffa280, aRawPtr=0x5a5a5a5a5a5a5a5a)
    at ../../dist/include/nsCOMPtr.h:607
#1  0x00007ffff2ad7ed0 in HandleEvent (aEvent=0x7fffffffa370)
    at /home/jst/fast/work/tip/mozilla/view/src/nsView.cpp:158
#2  0x00007ffff353a5be in nsWindow::DispatchEvent (this=0x7fffd79a0d50, aEvent=
    0x7fffffffa370, aStatus=@0x7fffffffa3cc)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsWindow.cpp:611
#3  0x00007ffff353a6a5 in nsWindow::OnDestroy (this=0x7fffd79a0d50)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsWindow.cpp:636
#4  0x00007ffff353b00d in nsWindow::Destroy (this=0x7fffd79a0d50)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsWindow.cpp:849
#5  0x00007ffff353a9df in nsWindow::DestroyChildWindows (this=0x7ffff6d55930)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsWindow.cpp:724
#6  0x00007ffff353af3d in nsWindow::Destroy (this=0x7ffff6d55930)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsWindow.cpp:830
#7  0x00007ffff2ad84c6 in nsView::DestroyWidget (this=0x7fffd9429980)
    at /home/jst/fast/work/tip/mozilla/view/src/nsView.cpp:299
#8  0x00007ffff2ad831d in nsView::~nsView (this=0x7fffd9429980, 
    __in_chrg=<value optimized out>)
    at /home/jst/fast/work/tip/mozilla/view/src/nsView.cpp:270
#9  0x00007ffff2ad83b2 in nsView::~nsView (this=0x7fffd9429980, 
    __in_chrg=<value optimized out>)
    at /home/jst/fast/work/tip/mozilla/view/src/nsView.cpp:277
#10 0x00007ffff2ad8634 in nsIView::Destroy (this=0x7fffd9429980)
    at /home/jst/fast/work/tip/mozilla/view/src/nsView.cpp:342
#11 0x00007ffff2498ee7 in nsFrame::DestroyFrom (this=0x7fffd941bc00, 
    aDestructRoot=0x7fffd941bc00)
    at /home/jst/fast/work/tip/mozilla/layout/generic/nsFrame.cpp:581
#12 0x00007ffff25112bb in nsSplittableFrame::DestroyFrom (this=0x7fffd941bc00, 
    aDestructRoot=0x7fffd941bc00)
    at /home/jst/fast/work/tip/mozilla/layout/generic/nsSplittableFrame.cpp:75
#13 0x00007ffff249035c in nsContainerFrame::DestroyFrom (this=0x7fffd941bc00, 
    aDestructRoot=0x7fffd941bc00)
    at /home/jst/fast/work/tip/mozilla/layout/generic/nsContainerFrame.cpp:290
#14 0x00007ffff253a128 in ViewportFrame::DestroyFrom (this=0x7fffd941bc00, 
    aDestructRoot=0x7fffd941bc00)
    at /home/jst/fast/work/tip/mozilla/layout/generic/nsViewportFrame.cpp:74
#15 0x00007ffff2397630 in nsIFrame::Destroy (this=0x7fffd941bc00)
---Type <return> to continue, or q <return> to quit---
    at ../../../mozilla/layout/base/../generic/nsIFrame.h:576
#16 0x00007ffff23e5c8d in nsFrameManager::Destroy (this=0x7fffd9415838)
    at /home/jst/fast/work/tip/mozilla/layout/base/nsFrameManager.cpp:258
#17 0x00007ffff241103e in PresShell::Destroy (this=0x7fffd9415800)
    at /home/jst/fast/work/tip/mozilla/layout/base/nsPresShell.cpp:1273
#18 0x00007ffff23dd83d in DocumentViewerImpl::DestroyPresShell (this=
    0x7ffff6d66360)
    at /home/jst/fast/work/tip/mozilla/layout/base/nsDocumentViewer.cpp:4332
#19 0x00007ffff23d354f in DocumentViewerImpl::Destroy (this=0x7ffff6d66360)
    at /home/jst/fast/work/tip/mozilla/layout/base/nsDocumentViewer.cpp:1665
#20 0x00007ffff31c2b85 in nsDocShell::Destroy (this=0x7fffd9580800)
    at /home/jst/fast/work/tip/mozilla/docshell/base/nsDocShell.cpp:4596
#21 0x00007ffff3289aea in nsXULWindow::Destroy (this=0x7fffd99e6570)
    at /home/jst/fast/work/tip/mozilla/xpfe/appshell/src/nsXULWindow.cpp:529
#22 0x00007ffff3297ad0 in nsWebShellWindow::Destroy (this=0x7fffd99e6570)
    at /home/jst/fast/work/tip/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:784
#23 0x00007ffff32967a1 in nsWebShellWindow::HandleEvent (aEvent=0x7fffffffae70)
    at /home/jst/fast/work/tip/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:406
#24 0x00007ffff353a5be in nsWindow::DispatchEvent (this=0x7ffff6d554e0, aEvent=
    0x7fffffffae70, aStatus=@0x7fffffffaecc)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsWindow.cpp:611
#25 0x00007ffff353ea5c in nsWindow::OnDeleteEvent (this=0x7ffff6d554e0, 
    aWidget=0x7fffd99e6680, aEvent=0x7fffcb7b5c10)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsWindow.cpp:2448
#26 0x00007ffff3547220 in delete_event_cb (widget=0x7fffd99e6680, event=
    0x7fffcb7b5c10)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsWindow.cpp:5745
#27 0x00007fffeeb08223 in ?? () from /usr/lib64/libgtk-x11-2.0.so.0
#28 0x00007ffff05d103e in g_closure_invoke () from /lib64/libgobject-2.0.so.0
#29 0x00007ffff05e1e87 in ?? () from /lib64/libgobject-2.0.so.0
#30 0x00007ffff05eb555 in g_signal_emit_valist ()
   from /lib64/libgobject-2.0.so.0
#31 0x00007ffff05eb983 in g_signal_emit () from /lib64/libgobject-2.0.so.0
#32 0x00007fffeec3faef in ?? () from /usr/lib64/libgtk-x11-2.0.so.0
#33 0x00007fffeeb0634d in gtk_main_do_event ()
   from /usr/lib64/libgtk-x11-2.0.so.0
#34 0x00007fffee22ca8c in ?? () from /usr/lib64/libgdk-x11-2.0.so.0
---Type <return> to continue, or q <return> to quit---
#35 0x00007ffff00f9e33 in g_main_context_dispatch ()
   from /lib64/libglib-2.0.so.0
#36 0x00007ffff00fa610 in ?? () from /lib64/libglib-2.0.so.0
#37 0x00007ffff00fa8ad in g_main_context_iteration ()
   from /lib64/libglib-2.0.so.0
#38 0x00007ffff354bcd7 in nsAppShell::ProcessNextNativeEvent (this=
    0x7fffde965900, mayWait=false)
    at /home/jst/fast/work/tip/mozilla/widget/src/gtk2/nsAppShell.cpp:144
#39 0x00007ffff3571344 in nsBaseAppShell::DoProcessNextNativeEvent (this=
    0x7fffde965900, mayWait=false)
    at /home/jst/fast/work/tip/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:171
#40 0x00007ffff357173a in nsBaseAppShell::OnProcessNextEvent (this=
    0x7fffde965900, thr=0x7ffff6d21aa0, mayWait=false, recursionDepth=0)
    at /home/jst/fast/work/tip/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:312
#41 0x00007ffff38ac9a2 in nsThread::ProcessNextEvent (this=0x7ffff6d21aa0, 
    mayWait=false, result=0x7fffffffb72f)
    at /home/jst/fast/work/tip/mozilla/xpcom/threads/nsThread.cpp:595
#42 0x00007ffff383dd29 in NS_ProcessNextEvent_P (thread=0x7ffff6d21aa0, 
    mayWait=false)
    at /home/jst/fast/work/tip/fb-dbg/xpcom/build/nsThreadUtils.cpp:245
#43 0x00007ffff36daf20 in mozilla::ipc::MessagePump::Run (this=0x7fffe413a9c0, 
    aDelegate=0x7ffff6dcaff0)
    at /home/jst/fast/work/tip/mozilla/ipc/glue/MessagePump.cpp:110
#44 0x00007ffff3912763 in MessageLoop::RunInternal (this=0x7ffff6dcaff0)
    at /home/jst/fast/work/tip/mozilla/ipc/chromium/src/base/message_loop.cc:208
#45 0x00007ffff39126f4 in MessageLoop::RunHandler (this=0x7ffff6dcaff0)
    at /home/jst/fast/work/tip/mozilla/ipc/chromium/src/base/message_loop.cc:201
#46 0x00007ffff3912685 in MessageLoop::Run (this=0x7ffff6dcaff0)
    at /home/jst/fast/work/tip/mozilla/ipc/chromium/src/base/message_loop.cc:175
#47 0x00007ffff35713cc in nsBaseAppShell::Run (this=0x7fffde965900)
    at /home/jst/fast/work/tip/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:189
#48 0x00007ffff32ad34c in nsAppStartup::Run (this=0x7fffde9928d0)
    at /home/jst/fast/work/tip/mozilla/toolkit/components/startup/nsAppStartup.c---Type <return> to continue, or q <return> to quit---
pp:228
#49 0x00007ffff209dfae in XRE_main (argc=3, argv=0x7fffffffe388, aAppData=
    0x7ffff6d1c2b0)
    at /home/jst/fast/work/tip/mozilla/toolkit/xre/nsAppRunner.cpp:3552
#50 0x0000000000401fb1 in do_main (exePath=
    0x7fffffffd150 "/home/jst/fast/work/tip/fb-dbg/dist/bin/libxpcom.so", argc=
    3, argv=0x7fffffffe388)
    at /home/jst/fast/work/tip/mozilla/browser/app/nsBrowserApp.cpp:198
#51 0x00000000004021c7 in main (argc=3, argv=0x7fffffffe388)
    at /home/jst/fast/work/tip/mozilla/browser/app/nsBrowserApp.cpp:281
(gdb) up
#1  0x00007ffff2ad7ed0 in HandleEvent (aEvent=0x7fffffffa370)
    at /home/jst/fast/work/tip/mozilla/view/src/nsView.cpp:158
158	    nsCOMPtr<nsIViewManager> vm = view->GetViewManager();
(gdb) p view
$1 = (nsView *) 0x7fffcce1aca0
(gdb) p *view
$2 = {
  <nsIView> = {
    _vptr.nsIView = 0x5a5a5a5a5a5a5a5a, 
    mViewManager = 0x5a5a5a5a5a5a5a5a, 
    mParent = 0x5a5a5a5a5a5a5a5a, 
    mWindow = 0x5a5a5a5a5a5a5a5a, 
...
Comment 227 Josh Aas 2011-11-17 07:20:40 PST
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #226)
> Josh, looks like the crash I was seeing above still exists in the latest
> patch :(

Thanks, looking into it now.

> I also found another crash with these patches, so far I've only been able to
> trigger it with flashblock installed. Start firefox, open a new window, load
> http://cdn-static.viddler.com/flash/as3/simple-publisher.
> swf?key=cb852767&ref=www.google.com in it, close the window (w/o even
> clicking to play the flash content) and I crash with the following signature:

Interesting. I've seen this happen before (due to other bugs) when a plugin instance is not destroyed properly.
Comment 228 Chris Pearce (:cpearce) 2011-11-17 13:21:41 PST
I noticed that with patches plugin-42 and widget-8 applied, on Windows (didn't test other platforms) Pandora.com doesn't load. Eventually Pandora.com displays a "taking too long to load, try refreshing" page instead.

The try builds with patches plugin-42 and widget-8 applied that I was testing with:
https://tbpl.mozilla.org/?tree=Try&rev=39cd6c7bb67c
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-39cd6c7bb67c/

STR:
1. Download & run build from link above.
2. Navigate to http://pandora.com.
3. Observe pandora's music player page never loads, eventually a "taking too long to load" warning page is shown.
Comment 229 Ed Morley [:emorley] 2011-11-21 19:07:39 PST
Part 1:
https://hg.mozilla.org/mozilla-central/rev/ec384df0ce77
Comment 230 Josh Aas 2011-11-29 08:12:11 PST
Created attachment 577622 [details] [diff] [review]
plugin fix v3.2

The main difference between this revision and the last one is that this simplifies the instantiation path so that all instance instantiations use the same code path. This fixes a number of potential problems and makes the code much easier to understand.

The only known problem with this patch is Pandora on Windows. All tests pass.
Comment 231 Jesse Ruderman 2011-11-29 16:26:10 PST
> The only known problem with this patch is Pandora on Windows. All tests pass.

Sounds like we're missing a test ;)
Comment 232 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-29 17:59:50 PST
Created attachment 577821 [details]
Log of pandora loading w/o this patch.
Comment 233 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-29 18:09:49 PST
Created attachment 577822 [details]
Log of pandora loading with this patch.

This is a log of failing to load pandora.com with this patch attached (on windows). The high-level difference seems to be that flXHR.swf is never loaded in the failing case. The root of that problem is not yet something I undersand, or had much time to dig into yet, but it's likely related to the assertions about receiving nonqueued messages during sync IPC etc, i.e.:

0[6d5418]: ###!!! ASSERTION: Received "nonqueued" message 49360 during a synchronous IPC message for window 723470 ("MozillaDropShadowWindowClass"), sending it to DefWindowProc instead of the normal window procedure.: 'Error', file c:/Users/jst/work/tip/fb-dbg/ipc/glue/../../../mozilla/ipc/glue/WindowsMessageLoop.cpp, line 328

bsmedberg, jimm, any thoughts off the top of your heads as to what could be the cause of assertions like these?
Comment 234 Jim Mathies [:jimm] 2011-11-30 03:10:26 PST
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #233)
> Created attachment 577822 [details]
> Log of pandora loading with this patch.
> 
> This is a log of failing to load pandora.com with this patch attached (on
> windows). The high-level difference seems to be that flXHR.swf is never
> loaded in the failing case. The root of that problem is not yet something I
> undersand, or had much time to dig into yet, but it's likely related to the
> assertions about receiving nonqueued messages during sync IPC etc, i.e.:
> 
> 0[6d5418]: ###!!! ASSERTION: Received "nonqueued" message 49360 during a
> synchronous IPC message for window 723470 ("MozillaDropShadowWindowClass"),
> sending it to DefWindowProc instead of the normal window procedure.:
> 'Error', file
> c:/Users/jst/work/tip/fb-dbg/ipc/glue/../../../mozilla/ipc/glue/
> WindowsMessageLoop.cpp, line 328
> 
> bsmedberg, jimm, any thoughts off the top of your heads as to what could be
> the cause of assertions like these?

Those errors are the result of unhandled windowing events we get in a hook we have set that prevents win event delivery while we are waiting on an ipc message response from the plugin.

The message in question has the value 0xC0D0 which means it was created via RegisterWindowMessage.  We use these internally in a few places including dealing with some focus issues with windowed plugins, and in our win app shell message pumping. It might be worth breaking out on this in WindowsMessageLoop's hook to see who sent it. But overall since it's an internal message we manage I doubt it has anything to do with Pandora's failure to load.
Comment 235 Josh Aas 2011-12-01 11:24:43 PST
Created attachment 578336 [details] [diff] [review]
plugin fix v3.3

Updated to current trunk.
Comment 236 Josh Aas 2011-12-01 14:51:30 PST
Created attachment 578409 [details] [diff] [review]
plugin fix v3.4

Last patch I uploaded was missing some test files.

I've also discovered that this bug reported by cpearce has regressed again in the latest patch.

http://pearce.org.nz/full-screen/90268-bug.html

That needs to be fixed and a test for it added in addition to a fix for the Pandora issue on Windows.
Comment 237 Josh Aas 2011-12-01 19:33:53 PST
Created attachment 578479 [details] [diff] [review]
plugin fix v3.5

Fixes the regression documented in bug 707052. There is an automated test in that bug which works locally but it's hard to do in a reliable way, one that we'd want to check in. I'll keep thinking about it.
Comment 238 Benjamin Smedberg [:bsmedberg] 2011-12-05 10:26:05 PST
The relevant exceptions on Pandora appear to be:

ExternalEventService: can't communicate with secretary
http://www.pandora.com/script.combined.js?v=93218408
Line 511
ExternalEventService: can't communicate with lifter
http://www.pandora.com/script.combined.js?v=93218408
Line 511

Both the secretary and the lifter are <object> elements which are width="0" height="0" elements dynamically inserted into the <body> after it is constructed. Their computed style according to Firebug is:

width: 0; height: 0; top: -1px; left: -1px; position: absolute; display: block; visibility: visible;

I'm trying to figure out whether we have plugin instances for these objects or not...
Comment 239 Benjamin Smedberg [:bsmedberg] 2011-12-06 10:02:47 PST
I have spent a couple days trying to debug the Pandora scripts without success, partly because they are minified and partly because the timeouts mess up any breakpoints in the JS debugger. I'm trying to contact Pandora developers now to see if we can get some help understanding the failure mode or similar kinds of help.
Comment 240 Josh Aas 2011-12-26 15:14:35 PST
Created attachment 584353 [details] [diff] [review]
plugin fix v3.6

Updated to current trunk. Includes a fix for an assertion on Mac OS X re: layer-based painting. Everything passes on try server but we still need a fix for Pandora on Windows once we figure out what is going on there.
Comment 241 Benjamin Smedberg [:bsmedberg] 2011-12-30 08:29:22 PST
http://hg.mozilla.org/users/bsmedberg_mozilla.com/jPlayer-testcase/raw-file/default/index.html contains a somewhat-minimized testcase for the pandora issue, which is was reduced to an issue where the jPlayer SWF file never gets initialized (the class constructor is never called here: http://hg.mozilla.org/users/bsmedberg_mozilla.com/jPlayer-testcase/file/969a31ffe2ec/jQuery.jPlayer.2.1.0.source/Jplayer.as#l66

From what I can tell when debugging, in the nonpatched case the constructor is called during or after the NPP_DestroyStream for the .swf file stream.

In the patched case, we deliver the .swf file stream correctly, and then we immediately deliver the .swf file stream *again*. I haven't figured out yet why we're delivering it twice, but I suspect that this is what's confusing Flash.
Comment 242 Benjamin Smedberg [:bsmedberg] 2011-12-30 09:41:38 PST
The two streams are initialized at these stacks:

>	xul.dll!nsPluginStreamListenerPeer::nsPluginStreamListenerPeer()  Line 312	C++
 	xul.dll!nsPluginHost::NewEmbeddedPluginStreamListener(aURL=0x0995dae8, aContent=0x09392c18, aInstance=0x00000000, aListener=0x09392c54)  Line 3281	C++
 	xul.dll!nsPluginHost::InstantiatePluginForChannel(aChannel=0x0995e058, aContent=0x09392c18, aListener=0x09392c54)  Line 966	C++
 	xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x0995e058, aContext=0x00000000)  Line 874	C++
 	xul.dll!nsBaseChannel::OnStartRequest(request=0x0986e230, ctxt=0x00000000)  Line 730	C++
 	xul.dll!nsInputStreamPump::OnStateStart()  Line 441	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(stream=0x0995e118)  Line 397	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 115	C++
 	xul.dll!nsThread::ProcessNextEvent(mayWait=false, result=0x002ad113)  Line 625	C++

>	xul.dll!nsPluginStreamListenerPeer::nsPluginStreamListenerPeer()  Line 312	C++
 	xul.dll!nsPluginHost::NewEmbeddedPluginStreamListener(aURL=0x0995dae8, aContent=0x00000000, aInstance=0x09622cb8, aListener=0x002ac9a0)  Line 3281	C++
 	xul.dll!nsPluginHost::NewEmbeddedPluginStream(aURL=0x0995dae8, aContent=0x00000000, aInstance=0x09622cb8)  Line 3313	C++
 	xul.dll!nsPluginHost::InstantiateEmbeddedPlugin(aMimeType=0x09606640, aURL=0x0995dae8, aContent=0x09392c18, aOwner=0x09392c88)  Line 1108	C++
 	xul.dll!nsObjectLoadingContent::InstantiatePluginInstance(aMimeType=0x09606640, aURI=0x0995dae8)  Line 636	C++
 	xul.dll!nsPluginStreamListenerPeer::OnStartRequest(request=0x0995e058, aContext=0x00000000)  Line 638	C++
 	xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x0995e058, aContext=0x00000000)  Line 897	C++
 	xul.dll!nsBaseChannel::OnStartRequest(request=0x0986e230, ctxt=0x00000000)  Line 730	C++
 	xul.dll!nsInputStreamPump::OnStateStart()  Line 441	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(stream=0x0995e118)  Line 397	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 115	C++
 	xul.dll!nsThread::ProcessNextEvent(mayWait=false, result=0x002ad113)  Line 625	C++
Comment 243 Benjamin Smedberg [:bsmedberg] 2011-12-30 09:43:16 PST
And http://hg.mozilla.org/users/bsmedberg_mozilla.com/bug90268-init-testcase/ has the most-reduced testcase (no jQuery/jPlayer, dirt-simple SWF).
Comment 244 Steve Heffernan 2012-01-04 17:21:48 PST
Thanks for the work on this bug. Keep it up!

As a potential workaround, has anyone tried loading the plugin (flash) inside an iFrame and setting it to 100% width/height of the iframe, then changing the size of the iframe instead? In initial tests, that seems to get around it for me.
Comment 245 Josh Aas 2012-01-09 23:22:47 PST
Created attachment 587250 [details] [diff] [review]
plugin fix v3.7

Updated to current trunk.

I don't have access to a dev machine with Windows right now and I can't reproduce the Pandora using Benjamin's reduced test case on Mac OS X. I'll try reproducing on Linux next.
Comment 246 Josh Aas 2012-01-10 08:04:04 PST
I can't reproduce the Pandora bug using Benjamin's test case on Linux either.
Comment 247 Benjamin Smedberg [:bsmedberg] 2012-01-10 10:28:02 PST
Created attachment 587367 [details] [diff] [review]
Mochitest to ensure that we don't deliver the data="" data twice, rev. 1

This is a mochitest-style test for the "pandora issue" that should work on any platform. It tests that we only receive the <object data=""> data once, and it currently passes on m-c. So I'll just land it now after review.
Comment 248 Benjamin Smedberg [:bsmedberg] 2012-01-12 09:44:52 PST
http://hg.mozilla.org/mozilla-central/rev/baa88d873a77 (the multiple-stream test) landed yesterday.
Comment 249 Matt Brubeck (:mbrubeck) 2012-01-12 10:54:17 PST
https://hg.mozilla.org/mozilla-central/rev/baa88d873a77
Comment 250 Josh Aas 2012-01-12 10:56:23 PST
This isn't fixed, that was just a test that already passes.
Comment 251 Josh Aas 2012-01-12 12:54:50 PST
Created attachment 588161 [details] [diff] [review]
plugin fix v3.8

Updated to current trunk and contains a stream fix for plugins that don't have object frames. This patch still has "the Pandora problem."
Comment 252 Josh Heidenreich 2012-01-12 14:19:58 PST
Is there a test case for the XStandard plugin?
http://www.xstandard.com/

Can one be created? Is it just a HTML file? Can both v2 and v3 be tested, on both windows and mac?
Comment 253 Vlad Alexander 2012-01-12 14:44:08 PST
This is a test page for XStandard plug-in:

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

The plug-in installers can be downloaded from:
http://xstandard.com/en/download/

Just tested using the following and this bug still persists using:
- Nightly 12.0a1 (2012-01-12) on Windows
- XStandard 3.0
- Test page above
Comment 254 Josh Aas 2012-01-12 21:30:57 PST
Created attachment 588307 [details] [diff] [review]
plugin fix v3.9

This includes a fix that should resolve the Pandora issues. Benjamin - can you test?

I'm going to keep testing but right now I'm not aware of any more problems with this patch. Requesting review again as I've made significant changes since the last time we did a review.
Comment 255 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-13 01:07:49 PST
Comment on attachment 588307 [details] [diff] [review]
plugin fix v3.9

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

::: content/base/src/nsObjectLoadingContent.h
@@ +402,5 @@
>      // Used to keep track of whether or not a plugin should be played.
>      // This is used for click-to-play plugins.
>      bool                        mShouldPlay : 1;
>  
> +    bool mSrcStreamLoadInitiated;

Document this
Comment 256 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-13 01:10:55 PST
My review here is not very good. I couldn't, for example, identify what you changed to fix Pandora.
Comment 257 Mikko Rantalainen 2012-01-13 04:25:47 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #256)
> My review here is not very good. I couldn't, for example, identify what you
> changed to fix Pandora.

From comparing v3.8 and v3.9 it seems that all the changes deal with mSrcStreamLoadInitiated so I agree that it should have a good documentation. I guess that it's a flag that prevents double initialization of stream loading. I'd bet it's related to this:

(Benjamin Smedberg  [:bsmedberg] from comment #241)
> In the patched case, we deliver the .swf file stream correctly, and then we
> immediately deliver the .swf file stream *again*. I haven't figured out yet
> why we're delivering it twice, but I suspect that this is what's confusing
> Flash.
Comment 258 Mardeg 2012-01-16 17:12:38 PST
Is there a link to a try build that we can test whether the Flash reload-on-scroll happening in http://www.techieinsider.com/news/14225/video-windows-phone-skype-microsoft/ is solved or still happens?
Comment 259 Justin Wood (:Callek) 2012-01-16 20:30:30 PST
Just pushed v3.9 to try, I decided to make it a m-c matching push.. (Mardeg pinged in IRC)

Thanks for your try submission (http://hg.mozilla.org/try/pushloghtml?changeset=be8e5f8caff3).  It's the best!

Watch https://tbpl.mozilla.org/?tree=Try&rev=be8e5f8caff3 for your results to come in.

Builds and logs will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Callek@gmail.com-be8e5f8caff3.

This directory won't be created until the first builds are uploaded, so please be patient.
Comment 260 TinyButStrong 2012-01-17 03:33:01 PST
Confirming on Seven x64 here, bug fixed on try build above.
Comment 261 Josh Heidenreich 2012-01-17 15:14:15 PST
I downloaded the Win, OSX and Lin builds from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Callek@gmail.com-be8e5f8caff3/

Tested the XStandard plugin on OSX (using http://xstandard.com/misc/mozilla/hide.htm) and it appears to work perfectly, no crashes whatsoever.

I will continue to test on other platforms in the coming days.
Comment 262 Josh Aas 2012-01-19 11:37:23 PST
Created attachment 589928 [details] [diff] [review]
plugin fix v4.0

Updated to current trunk.
Comment 263 Daniel 2012-01-20 06:39:29 PST
I'm a web developer and this has been following me around for quite a while, especially with Mozilla Firefox gradually increasing its market share.

I can confirm that the build under http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Callek@gmail.com-be8e5f8caff3. has fixed an issue I had, where a java applet contained within a jquery tab unloaded completely when I switched tabs. This did not occur in IE8/9.

Is this fix slated for release in Firefox 12 or is it possible to have this fixed in Firefox 10 or maybe even right now in Firefox 9? After all, this has seemingly been around for over a decade.
Comment 264 Justin Lebar (not reading bugmail) 2012-01-20 07:38:10 PST
> Is this fix slated for release in Firefox 12 or is it possible to have this fixed in Firefox 10 or 
> maybe even right now in Firefox 9? After all, this has seemingly been around for over a decade.

It looks like this may make FF12 (if it's checked in by the end of the month), but in any case we're almost certainly not going to backport to older versions.  This is a large change with high probability of regressions (see the regressions which happened the first time this was checked in), and we try to take only safe changes in Aurora and Beta.
Comment 265 Josh Aas 2012-01-20 11:50:42 PST
Given the risk that comes with this patch we won't be landing it near the end of any development cycle, so this will not be landed for Firefox 12. Hopefully it'll be ready to go near the start of the Firefox 13 dev cycle. We've got one more known issue to resolve, which is that Pandora fails to load on Windows.
Comment 266 Josh Aas 2012-01-23 18:22:29 PST
Created attachment 590969 [details] [diff] [review]
plugin fix v4.1

Includes a fix for Pandora. The solution was to allow an NPP_SetWindow call to succeed on instantiation even before we have an object frame.

I don't know of any remaining issues with this patch, but I'll keep testing with this last change.
Comment 267 Benjamin Smedberg [:bsmedberg] 2012-01-26 11:07:56 PST
Comment on attachment 590969 [details] [diff] [review]
plugin fix v4.1

I reviewed the interdiff between this and the previous patches and this appears correct. I still want to look over the entire patch to understand what's changing, but I don't think that should hold this bug up since it has already been fully reviewed.
Comment 268 Josh Aas 2012-01-27 08:20:16 PST
Created attachment 592143 [details] [diff] [review]
plugin fix v4.2

Updated to current trunk.
Comment 269 Josh Aas 2012-01-31 13:59:29 PST
pushed plugin fix v4.2 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/15b00ab7f22d
Comment 270 Gary [:streetwolf] 2012-02-02 07:13:12 PST
Shouldn't this be reopened due to all the recent reported problems with it?  Backing it off might be necessary unless a proper fix is forthcoming.
Comment 271 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-02-02 07:17:28 PST
This shouldn't be reopened unless the patch is backed out.  Regressions are tracked in other bugs.
Comment 272 ronhab 2012-05-13 03:54:45 PDT
*** Bug 752827 has been marked as a duplicate of this bug. ***
Comment 273 Andy 2012-05-17 15:53:13 PDT
We have a class that changes on a container that contains an ad banner. The style is changed depending on position of scrollbar. Basically is changes style property display: absolute to fixed and vice versa. Whenever the class name changes the flash banner refreshes which we dont want. We see this in FF 12 and this is still a problem. Why is this fixed?
Comment 274 Timothy Nikkel (:tnikkel) 2012-05-17 15:55:51 PDT
This bug didn't land for FF 12. It will first be in FF 13. You can tell this by the target milestone set on this bug (mozilla13). We mark bugs fixed when they land on mozilla-central (nightly builds).
Comment 275 Benjamin Smedberg [:bsmedberg] 2013-01-16 08:36:43 PST
*** Bug 335583 has been marked as a duplicate of this bug. ***
Comment 276 wolsen 2013-07-18 12:32:09 PDT
It appears this bug has been regressed in FF22. See jsfiddle http://jsfiddle.net/YMhfA/6/ for an example test case which shows the regression.
Comment 277 Timothy Nikkel (:tnikkel) 2013-07-18 12:38:39 PDT
If this problem is not the same as bug 889614 then I suggest you file a new bug for it.
Comment 278 wolsen 2013-07-18 14:57:15 PDT
ah indeed this does look the same as bug 889614, apologies on not finding that one
Comment 279 stiles.nathan 2015-09-04 05:53:23 PDT
Seems attachment above is still demonstrating this issue?
 reframe testcase - changing CSS display type (881 bytes, text/html)
2002-03-06 22:00 PST, Peter Lubczynski 

FF 40.0.3 Win 7 64

Note You need to log in before you can comment on or make changes to this bug.