Closed Bug 90268 Opened 23 years ago Closed 13 years ago

move plugins to content - plugins should withstand a reframe of the object frame

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla13

People

(Reporter: peterlubczynski-bugs, Assigned: jaas)

References

(Blocks 1 open bug, )

Details

(Keywords: testcase, topembed-)

Attachments

(6 files, 57 obsolete files)

881 bytes, text/html
Details
277 bytes, text/plain
Details
2.67 KB, text/html
Details
8.36 KB, patch
Details | Diff | Splinter Review
5.24 KB, patch
jaas
: review+
Details | Diff | Splinter Review
179.32 KB, patch
Details | Diff | Splinter Review
...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.
Blocks: 88998
rephrasing...
Summary: object frame should be able to withstand a reframe → plugins should withstand an reframe of the object frame
lemme try to tackle this for 0.9.4.
Assignee: av → waterson
Priority: -- → P2
Target Milestone: --- → mozilla0.9.4
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
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.
Keywords: edt0.9.4
are we close to a patch here?
minusing. 0.9.4 is not targeted for broad i18n distribution, this can wait.
Keywords: edt0.9.4edt0.9.4-, topembed
-->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
Assignee: waterson → av
Status: ASSIGNED → NEW
Hardware: PC → All
I don't think I can do it for 0.9.7. Moving.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.7 → mozilla0.9.8
waterson, did you make any progress on this bug while it was assigned to you?
Whiteboard: NO ETA
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.
Keywords: mozilla1.0
Target Milestone: mozilla0.9.8 → mozilla1.0
nominating nsbeta1
Keywords: nsbeta1
nsbeta1+ per ADT Triage, reassigning to peterl
Assignee: av → peterlubczynski
Status: ASSIGNED → NEW
Keywords: nsbeta1nsbeta1+
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.
Keywords: nsbeta1+nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.1
topembed+ because this is known to be a large issue affecting plugins
Keywords: topembedtopembed+
I think we need to figure out how to reparent the plugin's widget before this can be fixed. Marking depends on bug 135263.
Depends on: 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.
Whiteboard: NO ETA → [NO ETA][high risk]
*** Bug 78242 has been marked as a duplicate of this bug. ***
Blocks: 152927
Blocks: 89077
Keywords: mozilla1.0mozilla1.0-
moving plugins to content would increase stability and is scheduled for plugins 2.0
Summary: plugins should withstand an reframe of the object frame → move plugins to content - plugins should withstand an reframe of the object frame
Whiteboard: [NO ETA][high risk] → [NO ETA][high risk][PL2:P1]
Target Milestone: mozilla1.1alpha → mozilla1.2beta
No longer blocks: 152927
Blocks: 169951
*** Bug 173276 has been marked as a duplicate of this bug. ***
Blocks: grouper
Blocks: 180802
moving to 1.4 beta, plug-in branch work
Whiteboard: [NO ETA][high risk][PL2:P1] → [PL:BRANCH]
Target Milestone: mozilla1.2beta → mozilla1.4beta
Blocks: 136927
Blocks: 158670
per topembed+ triage: topembed-, Future
Keywords: topembed+topembed-
Target Milestone: mozilla1.4beta → Future
Blocks: 203401
No longer blocks: 169951
Summary: move plugins to content - plugins should withstand an reframe of the object frame → move plugins to content - plugins should withstand a reframe of the object frame
Assignee: peterlubczynski-bugs → cbiesinger
Target Milestone: Future → ---
Blocks: 300659
*** 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.
Attached patch Proposed fix for the plugin unload/reload bug (obsolete) — — Splinter Review
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.
Flags: wanted1.9.1?
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).
Blocks: 468768
QA Contact: shrir → plugins
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.
Flags: wanted1.9.1? → wanted1.9.2?
Whiteboard: [PL:BRANCH]
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.
Attachment #326389 - Attachment is obsolete: true
Attached file test page to display flash reset problem (obsolete) —
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.
Attachment #410848 - Attachment is obsolete: true
Attached file Page showing the flash reset problem (obsolete) —
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.
blocking2.0: --- → ?
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
Keywords: testcase
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.
Assignee: cbiesinger → justin.lebar+bug
blocking2.0: ? → final+
Reassigning to nobody. Per Shaver's talk tonight, this can't be a priority right now.
Assignee: justin.lebar+bug → nobody
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.
blocking2.0: final+ → -
Flags: wanted1.9.2?
I'm planning to work on this for after Gecko 2.0.0.
Assignee: nobody → joshmoz
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.
Blocks: 614825
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.
blocking2.0: - → ?
This cannot block. It is multiple man-months of effort to fix and nobody has stepped up.
blocking2.0: ? → ---
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!
Depends on: 660721
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.
Attached patch fix v0.4 (obsolete) — — Splinter Review
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.
Attached patch fix v0.6 (obsolete) — — Splinter Review
Adds support for Linux, and re-framing works now.
Attachment #538440 - Attachment is obsolete: true
Attached patch fix v0.6 + windows build fix (obsolete) — — Splinter Review
This version of the patch builds successfully on Windows too.
Attachment #539517 - Attachment is obsolete: true
Attached patch fix v0.7 (obsolete) — — Splinter Review
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.
Attachment #539607 - Attachment is obsolete: true
Depends on: 665996
Attached patch fix v0.75 (obsolete) — — Splinter Review
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.
Attachment #539735 - Attachment is obsolete: true
(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
Attached patch fix v0.75 updated to latest m-c (obsolete) — — Splinter Review
Attachment #541965 - Attachment is obsolete: true
Attached patch fix v0.76 (obsolete) — — Splinter Review
Fixes a memory leak, updates to latest m-c.
Attachment #542143 - Attachment is obsolete: true
Attached patch fix v0.8 (obsolete) — — Splinter Review
Makes windowed plugins work on Linux and Windows. Fixes a number of test failures.
Attachment #545206 - Attachment is obsolete: true
Attached patch fix v0.8, updated to current m-c (obsolete) — — Splinter Review
Attachment #545832 - Attachment is obsolete: true
Depends on: 674223
Depends on: 674224
Depends on: 675005
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.
Attached patch fix v0.8, updated to current m-c (obsolete) — — Splinter Review
Attachment #547669 - Attachment is obsolete: true
Attached patch fix v0.8, updated to current m-c (obsolete) — — Splinter Review
Attachment #551786 - Attachment is obsolete: true
Attached patch fix v0.82 (obsolete) — — Splinter Review
Includes a fix for some test failures. Still have to resolve more test failures.
Attachment #554544 - Attachment is obsolete: true
Attached patch fix v1.0 (obsolete) — — Splinter Review
Attachment #554760 - Attachment is obsolete: true
Attachment #554998 - Attachment description: fix v1.1 → fix v1.0
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.
Attachment #554998 - Flags: review?(roc)
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.
Attached patch fix v1.1 (obsolete) — — Splinter Review
Includes fixes for some of what roc already pointed out, will do more tomorrow.
Attachment #554998 - Attachment is obsolete: true
Attachment #554998 - Flags: review?(roc)
Attachment #554998 - Flags: review?(cbiesinger)
Attachment #555298 - Flags: review?(roc)
(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.
Attached patch fix v1.2 (obsolete) — — Splinter Review
More fixes, still have a couple of comments to address.
Attachment #555298 - Attachment is obsolete: true
Attachment #555298 - Flags: review?(roc)
Attached patch widget fix v1.3 (obsolete) — — Splinter Review
Attachment #555626 - Attachment is obsolete: true
Attached patch other fix v1.3 (obsolete) — — Splinter Review
Attachment #555832 - Flags: review?(roc)
Attachment #555834 - Flags: review?(roc)
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.
Attachment #555832 - Flags: review?(roc)
Attachment #555832 - Flags: review?(karlt)
Attachment #555832 - Flags: review+
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.
Attachment #555832 - Flags: review?(karlt) → review-
Attached patch widget fix v1.4 (obsolete) — — Splinter Review
Attachment #555832 - Attachment is obsolete: true
Attachment #556221 - Flags: review?(karlt)
Attached patch other fix v1.4 (obsolete) — — Splinter Review
Attachment #555834 - Attachment is obsolete: true
Attachment #555834 - Flags: review?(roc)
Attachment #556404 - Flags: review?(roc)
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).
Attachment #556221 - Flags: review?(karlt) → review+
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.
Attached patch widget fix v1.5 (obsolete) — — Splinter Review
Attachment #556221 - Attachment is obsolete: true
Attached patch other fix v1.5 (obsolete) — — Splinter Review
Attachment #556404 - Attachment is obsolete: true
Attachment #556404 - Flags: review?(roc)
Attachment #556469 - Flags: review?(roc)
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.)
Attachment #556469 - Flags: review?(roc) → review+
Attached patch other fix v1.6 (obsolete) — — Splinter Review
Attachment #556469 - Attachment is obsolete: true
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Depends on: 683007
Josh Aas, Thank You!!!
some Flash content is no longer loaded after this checkin. http://forums.mozillazine.org/viewtopic.php?p=11191871#p11191871
Blocks: 682951
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 :-)
No longer blocks: 682951
Depends on: 682951
Depends on: 683059
Depends on: 683084
I think we should probably back this out now (while it's still easy to do so) and fix these regressions before relanding.
Attached patch other fix v1.7 (obsolete) — — Splinter Review
Updated to current trunk, no further fixes.
Attachment #556476 - Attachment is obsolete: true
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.
Attached patch other fix v1.8 (obsolete) — — Splinter Review
Mainly improvements to code paths leading to NPP_SetWindow.
Attachment #557076 - Attachment is obsolete: true
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
Attached patch widget fix v1.6 (obsolete) — — Splinter Review
Updated to current trunk.
Attachment #556468 - Attachment is obsolete: true
Attached patch other fix v1.9 (obsolete) — — Splinter Review
Updated to current trunk with a few minor changes.
Attachment #558456 - Attachment is obsolete: true
Attached patch other fix v2.0 (obsolete) — — Splinter Review
Nice catch in comment 191, Chris. Thanks. This patch has a fix with a test.
Attachment #562160 - Attachment is obsolete: true
Attached patch other fix v2.1 (obsolete) — — Splinter Review
This includes a number of fixes, including a fix for Google Earth. Problem is, now Netflix doesn't work on Mac OS X.
Attachment #562343 - Attachment is obsolete: true
Target Milestone: mozilla9 → mozilla10
Attached patch widget fix v1.7 (obsolete) — — Splinter Review
Updated to current trunk.
Attachment #562159 - Attachment is obsolete: true
Attached patch other fix v2.2 (obsolete) — — Splinter Review
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.
Attachment #562982 - Attachment is obsolete: true
Attached patch other fix v2.3 (obsolete) — — Splinter Review
Fixes a number of test failures.
Attachment #565011 - Attachment is obsolete: true
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.
This might be a silly question, but why does Bug 545812 depend on this?
Sorry, my bad, should have been 684618.
See comment 191 for the test case, looks resolved but we are trying to figure out if it will land.
Attached patch widget fix v1.8 (obsolete) — — Splinter Review
Updated to current m-c.
Attachment #565004 - Attachment is obsolete: true
Attached patch other fix v2.4 (obsolete) — — Splinter Review
Updated to current m-c.
Attachment #565860 - Attachment is obsolete: true
(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.
Attached patch other fix v2.5 (obsolete) — — Splinter Review
Updated to current m-c.
Attachment #568303 - Attachment is obsolete: true
Attached file An other way to get the bug —
Get the bug just by changing the css positioning of the container.
Attached patch other fix v2.6 (obsolete) — — Splinter Review
Attachment #568420 - Attachment is obsolete: true
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 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.
Thanks Karl! Very helpful, I have a fix which will be in my next patch.
Attached patch widget fix v1.9 (obsolete) — — Splinter Review
Attachment #568302 - Attachment is obsolete: true
Attached patch plugin fix v2.7 (obsolete) — — Splinter Review
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).
Attachment #569455 - Attachment is obsolete: true
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
Depends on: 697651
Updating attachment 410850 [details] to use a url that is not 404.
Attachment #410850 - Attachment is obsolete: true
Changing Target Milestone to 11 based on comment 207
Target Milestone: mozilla10 → mozilla11
Attached patch plugin fix v2.8 (obsolete) — — Splinter Review
Updated to current m-c. I'll be removing initialization for display:none plugins next, per the discussion in bug 697651.
Attachment #569798 - Attachment is obsolete: true
Attached patch plugin fix v2.9 (obsolete) — — Splinter Review
Removes support for instantiating display:none instances. Also fixes a crash I've been seeing once in a while and finally tracked down.
Attachment #571673 - Attachment is obsolete: true
The latest patch introduces some new test failures, shouldn't be hard to fix though. Patch coming up soon.
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.
Attached patch widget fix v2.0 — — Splinter Review
Updated to current m-c.
Attachment #569797 - Attachment is obsolete: true
Attached patch plugin fix 3.0 (obsolete) — — Splinter Review
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.
Attachment #571752 - Attachment is obsolete: true
Attached patch plugin fix 3.1 (obsolete) — — Splinter Review
Fixes test failures. The only reliably failing test remaining is "test_pluginstream_err.html" on Windows and Linux.
Attachment #574310 - Attachment is obsolete: true
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, ...
(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.
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.
Attached patch plugin fix v3.2 (obsolete) — — Splinter Review
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.
Attachment #574784 - Attachment is obsolete: true
> The only known problem with this patch is Pandora on Windows. All tests pass. Sounds like we're missing a test ;)
Attached file Log of pandora loading w/o this patch. (obsolete) —
Attached file Log of pandora loading with this patch. (obsolete) —
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?
(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.
Attached patch plugin fix v3.3 (obsolete) — — Splinter Review
Updated to current trunk.
Attachment #577622 - Attachment is obsolete: true
Attached patch plugin fix v3.4 (obsolete) — — Splinter Review
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.
Attachment #578336 - Attachment is obsolete: true
Depends on: 707052
Attached patch plugin fix v3.5 (obsolete) — — Splinter Review
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.
Attachment #578409 - Attachment is obsolete: true
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...
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.
Target Milestone: mozilla11 → ---
Depends on: 713552
Attached patch plugin fix v3.6 (obsolete) — — Splinter Review
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.
Attachment #578479 - Attachment is obsolete: true
Blocks: 713632
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.
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++
And http://hg.mozilla.org/users/bsmedberg_mozilla.com/bug90268-init-testcase/ has the most-reduced testcase (no jQuery/jPlayer, dirt-simple SWF).
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.
Attached patch plugin fix v3.7 (obsolete) — — Splinter Review
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.
Attachment #584353 - Attachment is obsolete: true
Attachment #587250 - Attachment is patch: true
I can't reproduce the Pandora bug using Benjamin's test case on Linux either.
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.
Attachment #587367 - Flags: review?(joshmoz)
Attachment #587367 - Flags: review?(joshmoz) → review+
http://hg.mozilla.org/mozilla-central/rev/baa88d873a77 (the multiple-stream test) landed yesterday.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
This isn't fixed, that was just a test that already passes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla12 → ---
No longer blocks: 89077
Attached patch plugin fix v3.8 (obsolete) — — Splinter Review
Updated to current trunk and contains a stream fix for plugins that don't have object frames. This patch still has "the Pandora problem."
Attachment #587250 - Attachment is obsolete: true
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?
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
Attached patch plugin fix v3.9 (obsolete) — — Splinter Review
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.
Attachment #577821 - Attachment is obsolete: true
Attachment #577822 - Attachment is obsolete: true
Attachment #588161 - Attachment is obsolete: true
Attachment #588307 - Flags: review?
Attachment #588307 - Flags: review? → review?(roc)
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
Attachment #588307 - Flags: review?(roc) → review+
My review here is not very good. I couldn't, for example, identify what you changed to fix Pandora.
(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.
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?
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.
Confirming on Seven x64 here, bug fixed on try build above.
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.
Attached patch plugin fix v4.0 (obsolete) — — Splinter Review
Updated to current trunk.
Attachment #588307 - Attachment is obsolete: true
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.
> 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.
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.
Attached patch plugin fix v4.1 (obsolete) — — Splinter Review
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.
Attachment #589928 - Attachment is obsolete: true
Attachment #590969 - Flags: review?(benjamin)
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.
Attachment #590969 - Flags: review?(benjamin) → review+
Attached patch plugin fix v4.2 — — Splinter Review
Updated to current trunk.
Attachment #590969 - Attachment is obsolete: true
pushed plugin fix v4.2 to mozilla-central http://hg.mozilla.org/mozilla-central/rev/15b00ab7f22d
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 723154
Depends on: 723291
Depends on: 723379
Depends on: 519752
Depends on: 285982
Depends on: 723473
Depends on: 723476
Depends on: 723503
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.
This shouldn't be reopened unless the patch is backed out. Regressions are tracked in other bugs.
Depends on: 723520
Depends on: 723523
Depends on: 723133
Depends on: 723529
Depends on: 723558
No longer depends on: 723503
Depends on: 723856
No longer depends on: 723856
Depends on: 724248
Depends on: 724250
Depends on: 724532
Depends on: 724717
Depends on: 724812
Depends on: 725279
Depends on: 723217
Depends on: 725917
Blocks: 633985
Depends on: 726734
Depends on: 728672
Depends on: 737433
Depends on: 724781
Depends on: 736998
Depends on: 753251
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?
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).
Depends on: 719117
Depends on: 776451
Depends on: 784131
Depends on: 788900
Depends on: 825188
Depends on: 767673
Depends on: 865609
Depends on: 874027
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.
If this problem is not the same as bug 889614 then I suggest you file a new bug for it.
ah indeed this does look the same as bug 889614, apologies on not finding that one
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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: