...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.

rephrasing...

lemme try to tackle this for 0.9.4.

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.

I nominate this to edt0.9.4.

are we close to a patch here?

minusing. 0.9.4 is not targeted for broad i18n distribution, this can wait.

-->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

I don't think I can do it for 0.9.7. Moving.

waterson, did you make any progress on this bug while it was assigned to you?

BTW, Peter, fixing this bug could help to avoid the crash in bug 116232 even if that bug itself is not fixed.

Other than attachment 41015 [details] [diff] [review] (which is probably nice and bit-rotten by now), I didn't do anything.

This is a big effort, I do not think we can do it into 0.9.4.

nominating nsbeta1

nsbeta1+ per ADT Triage, reassigning to peterl

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.

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.

topembed+ because this is known to be a large issue affecting plugins

I think we need to figure out how to reparent the plugin's widget before this can be fixed. Marking depends on bug 135263.

Peter, is there any update on this one?

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.

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

moving plugins to content would increase stability and is scheduled for plugins 2.0

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

moving to 1.4 beta, plug-in branch work

per topembed+ triage: topembed-, Future

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

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

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.

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.

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... ;)

(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

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

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.

(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

(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...

> 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.

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?

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?

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

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.).

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.

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.

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.

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.

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?

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.

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.

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.

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.

> 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.

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.

(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).

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?

Is there any progress on this issue?

Any news on this bug? Really annoying...

This is a must-fix bug! But all i have is hope.

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?

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.

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 on attachment 410848 [details] test page to display flash reset problem Removing and adding a better test case.

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.

biesi, you took this bug, do you think you're likely to work on it in the foreseeable future?

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

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

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

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?

(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?!

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.

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>

(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.

(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.

@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.

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.

Reassigning to nobody. Per Shaver's talk tonight, this can't be a priority right now.

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...

(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.

(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?

(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.

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.

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

(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*.

Per the platform meeting, *this* bug is not going to block, but bug 449734 does so that app tabs and tabcandy and such work.

I'm planning to work on this for after Gecko 2.0.0.

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.

(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.

Hoping this gets fixed some day. :(

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...

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.

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.

This cannot block. It is multiple man-months of effort to fix and nobody has stepped up.

I am getting close to beginning work on this.

I am starting work on this today.

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...

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!

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.

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.

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.

Adds support for Linux, and re-framing works now.

This version of the patch builds successfully on Windows too.

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.

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.

(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 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.

(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.

(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.

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.

(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.

(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.

... and we know that the plugin instance will be destroyed or reparented before the browser native window is destroyed.

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

Fixes a memory leak, updates to latest m-c.

Makes windowed plugins work on Linux and Windows. Fixes a number of test failures.

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.

Includes a fix for some test failures. Still have to resolve more test failures.

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 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?

(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.

(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.

(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 :-).

(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.

Includes fixes for some of what roc already pointed out, will do more tomorrow.

(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.

(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).

(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).

(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.

(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.

(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.

More fixes, still have a couple of comments to address.

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 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 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 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 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 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 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 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 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.)

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."

(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.

pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/ab1cbc8a9d51 http://hg.mozilla.org/mozilla-central/rev/33031c875984

Josh Aas, Thank You!!!

some Flash content is no longer loaded after this checkin. http://forums.mozillazine.org/viewtopic.php?p=11191871#p11191871

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 :-)

I think we should probably back this out now (while it's still easy to do so) and fix these regressions before relanding.

Agreed, backed out. http://hg.mozilla.org/mozilla-central/rev/d5a0f6ad8bbd http://hg.mozilla.org/mozilla-central/rev/1a4a5fcf0a76 http://hg.mozilla.org/mozilla-central/rev/a6e7760f2f36 http://hg.mozilla.org/mozilla-central/rev/e6591ea9b27b

Updated to current trunk, no further fixes.

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?

(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.

Mainly improvements to code paths leading to NPP_SetWindow.

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!

AL, Bugzilla isn't a message board. Please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html (especially point 1.1)

(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.

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

Updated to current trunk.