The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla13

Status

()

Core
Plug-ins
P2
normal
RESOLVED FIXED
16 years ago
11 days ago

People

(Reporter: Peter Lubczynski, Assigned: Josh Aas)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {testcase, topembed-})

Trunk
mozilla13
testcase, topembed-
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 57 obsolete attachments)

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
Josh Aas
: review+
Details | Diff | Splinter Review
179.32 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
...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.
(Reporter)

Updated

16 years ago
Blocks: 88998
(Reporter)

Comment 1

16 years ago
rephrasing...
Summary: object frame should be able to withstand a reframe → plugins should withstand an reframe of the object frame

Comment 2

16 years ago
lemme try to tackle this for 0.9.4.
Assignee: av → waterson
Priority: -- → P2
Target Milestone: --- → mozilla0.9.4

Updated

16 years ago
Status: NEW → ASSIGNED

Updated

16 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Updated

16 years ago
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Updated

16 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.7

Comment 3

16 years ago
In embedding case, page with shockwave flash after font downloading
does not get rendered.
You need to manually hit reload button to render it right.

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

A couple of interesting points.
1) Only Korean sites with flash does not render at all
   after font downloading.
2) All other languages ( two Chinese and Japanese ) sites
   rendered strange.
   --> Flash overlapping the text in the middle of the page, etc.
3) Saving korean sites in local drive and loading it works fine and rendered
   correctly even after font downloading.
4) For other lang sites, loading from local drive still renderes the pages 
   right.

Comment 4

16 years ago
I nominate this to edt0.9.4.
Keywords: edt0.9.4

Comment 5

16 years ago
are we close to a patch here?

Comment 6

16 years ago
minusing. 0.9.4 is not targeted for broad i18n distribution, this can wait.
Keywords: edt0.9.4 → edt0.9.4-, topembed
(Reporter)

Comment 7

16 years ago
-->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

Comment 8

16 years ago
I don't think I can do it for 0.9.7. Moving.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.7 → mozilla0.9.8

Comment 9

15 years ago
waterson, did you make any progress on this bug while it was assigned to you?
Whiteboard: NO ETA

Comment 10

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

Comment 11

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

Comment 12

15 years ago
This is a big effort, I do not think we can do it into 0.9.4.
(Reporter)

Updated

15 years ago
Keywords: mozilla1.0

Updated

15 years ago
Target Milestone: mozilla0.9.8 → mozilla1.0
(Reporter)

Comment 13

15 years ago
nominating nsbeta1
Keywords: nsbeta1

Comment 14

15 years ago
nsbeta1+ per ADT Triage, reassigning to peterl
Assignee: av → peterlubczynski
Status: ASSIGNED → NEW
Keywords: nsbeta1 → nsbeta1+
(Reporter)

Comment 15

15 years ago
Created attachment 72926 [details]
reframe testcase - changing CSS display type

This testcase shows that changing the CSS display type of the EMBED tag or even
its surrounding tags causes a reframe and the plugin animation to restart.

Comment 16

15 years ago
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

Comment 17

15 years ago
topembed+ because this is known to be a large issue affecting plugins
Keywords: topembed → topembed+
(Reporter)

Comment 18

15 years ago
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

Comment 19

15 years ago
Peter, is there any update on this one?
(Reporter)

Comment 20

15 years ago
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]
(Reporter)

Comment 21

15 years ago
*** Bug 78242 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

15 years ago
Blocks: 152927
(Reporter)

Updated

15 years ago
Blocks: 89077
Keywords: mozilla1.0 → mozilla1.0-
(Reporter)

Comment 22

15 years ago
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
(Reporter)

Updated

15 years ago
No longer blocks: 152927

Updated

15 years ago
Blocks: 169951
*** Bug 173276 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Blocks: 176349
(Reporter)

Updated

15 years ago
Blocks: 180802

Comment 24

15 years ago
moving to 1.4 beta, plug-in branch work
Whiteboard: [NO ETA][high risk][PL2:P1] → [PL:BRANCH]
Target Milestone: mozilla1.2beta → mozilla1.4beta
(Reporter)

Updated

15 years ago
Blocks: 136927
(Reporter)

Updated

14 years ago
Blocks: 158670
(Reporter)

Comment 25

14 years ago
per topembed+ triage: topembed-, Future
Keywords: topembed+ → topembed-
Target Milestone: mozilla1.4beta → Future
(Reporter)

Updated

14 years ago
Blocks: 203401
(Reporter)

Updated

14 years ago
No longer blocks: 169951
Depends on: 1156
Blocks: 303888
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. ***

Comment 27

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

Comment 28

12 years ago
Here is another test-case:

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

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

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

As soon as they do this, the instance of the plug-in is destroyed and all data in the plug-in is lost.

Comment 29

12 years ago
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... ;)

Comment 31

11 years ago
(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. ***

Comment 33

11 years ago
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.
Blocks: 340465

Comment 34

11 years ago
(In reply to comment #33)
Marco, did you get any response to this? I'm not in a position to help you myself, as this is not my expertise. - I’m a front end and .net developer myself.

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

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

Best regards to all!
Phil Baines

Comment 35

11 years ago
(In reply to comment #34)
Phil, I didn't get any reply to my comment. Right now I am working around the problemen by setting visibility to hidden en size to 0px. Not very neat but is does the trick in my situation. But I would like to really solve it instead working around it...

Comment 36

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

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

Anyways, my solution:

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

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

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

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

Hope that helps.

Comment 37

10 years ago
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?
Duplicate of this bug: 391015

Updated

9 years ago
Duplicate of this bug: 406002

Updated

9 years ago
Duplicate of this bug: 411149

Comment 41

9 years ago
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?

Updated

9 years ago
Duplicate of this bug: 414029

Comment 43

9 years ago
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.).

Comment 45

9 years ago
Hi. I'm an NPAPI plugin developer and I encountered this unload/reload bug when debugging my plugin with the final FF3 release. Long story short, I was able to locate the place in the code where the bug happens. It's in nsObjectLoadingContent (content/base/src/nsObjectLoadingContent.cpp). The HasNewFrame method (called during reflow) schedules an event to asynchronously instantiate the plugin, but it does this even if the plugin has already been instantiated! Then when the event runs, nsObjectFrame (layout/generic/nsObjectFrame.cpp) gets told to instantiate an already-instantiated plugin and it destroys the existing instance first (in nsObjectFrame::PrepareInstanceOwner).

I have attached a patch made against FF3 that seems to fix the bug (at least on my one machine and for my one plugin). I simply changed nsObjectLoadingContent::HasNewFrame into a no-op. There are comments there suggesting that someone was supposed to do that already (they even refer to this very bug number), which is what suggests to me that this may be the correct fix. I'm not a Mozilla developer though and I don't fully understand this code so someone needs to take a closer look.

Comment 46

9 years ago
Created attachment 325884 [details] [diff] [review]
Proposed fix for the plugin unload/reload bug

Turns nsObjectLoadingContent::HasNewFrame into a no-op, instead of asynchronously (re-)instantiating the plugin. Needs attention from an expert in this area of the code.

Comment 47

9 years ago
Oh, I forgot to mention: if you're a webpage developer then another possible workaround is to call some method on the plugin's DOM element (or maybe just dereference a property?) right after any action that would normally trigger this bug. Doing that causes nsObjectLoadingContent::EnsureInstantiation to run, which as a side-effect will cancel the pending event that is responsible for the bug.

Comment 48

9 years ago
After some more testing, it looks like HasNewFrame has to do what it does in order for plugins to load on their own without waiting for something else to force them to, so the above patch is no good. It looks like my plugin's problems can be fixed by making HasNewFrame check to see if the plugin is already instantiated for that frame, which fixes an instantiation race when the instantiation gets done synchronously before the HasNewFrame() call occurs (which used to cause an unload/reload). However, that doesn't fix the problem with style.display = 'none'. I've attached a new patch to fix the race, but it should maybe be moved to a separate bug.

Comment 49

9 years ago
Created attachment 326389 [details] [diff] [review]
Don't asynchronously instantiate in HasNewFrame if already instantiated for that frame. Fixes a race where EnsureInstantiation would instantiate the plugin and then HasNewFrame would re-instantiate it
Attachment #325884 - Attachment is obsolete: true
Tristan, I think your problem is also covered by bug 438830. And note that this bug is not fixed, the part about moving plugins to content. We're part of the way there, but our layout code, as opposed to our content code, is still required for plugin loading, and that's what we're hopefully going to fix in this bug, at some point. Can you test if the patch in bug 438830 fixes the issue you're seeing?

Comment 51

9 years ago
Thanks Johnny, you're right, bug 438830 is the problem I'm seeing. Looking at the patch there, I don't even need to test it out, it's basically the same as mine. :) I'll post my findings there.

Comment 52

9 years ago
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.

Comment 54

9 years ago
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.
Duplicate of this bug: 451816
> What I was really trying to get across however is that the re-frame causing the
> flash component to be reset is still occurring in 3.01, and from what i
> understand it was believed to be fixed in the bug 438830 referenced above.

I'm afraid not :-(  Bug 438830, if I understand correctly, was only about a plugin getting loaded twice *during page load*, under certain conditions.

Comment 57

9 years ago
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

Updated

8 years ago
Duplicate of this bug: 479807

Comment 60

8 years ago
This one is a real bugger. We're using an embeded flash to handle upload with AS3 FileReference and when we lauch the dialog (hidding body overflow with document.body.style.overflow = 'hidden') showing download progress we reload the flash so we lose the FileReference and we get an upload failure since no files are selected anymore. Any updates on a patch release?

Comment 61

8 years ago
Is there any progress on this issue?

Comment 62

8 years ago
Any news on this bug? Really annoying...

Comment 63

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

Updated

8 years ago
Duplicate of this bug: 507182
Flags: wanted1.9.1? → wanted1.9.2?
Whiteboard: [PL:BRANCH]

Updated

8 years ago
Duplicate of this bug: 262354

Comment 66

8 years ago
Is there anything major that is holding this bug up?

Does the patch provided by Tristan Schmelcher work, and if it does, can this bug be fixed?

Comment 67

8 years ago
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.

Updated

8 years ago
Attachment #326389 - Attachment is obsolete: true

Comment 68

8 years ago
Created attachment 410848 [details]
test page to display flash reset problem

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

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

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

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

Firefox resets in all the scenarios.

Comment 69

8 years ago
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

Comment 70

8 years ago
Created attachment 410850 [details]
Page showing the flash reset problem

Comment 71

8 years ago
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.

Updated

7 years ago
Duplicate of this bug: 303888

Updated

7 years ago
blocking2.0: --- → ?
biesi, you took this bug, do you think you're likely to work on it in the foreseeable future?

Comment 74

7 years ago
Has to be said, viewing how this bug has been handled is a real eye opener. Its a complete blocker and an issue that only FF suffers from that has, and will hold back many web development projects, including the one I am working on... and here I am reading this log... 9 years guys, ... wow, let me guess, the next guy that takes this one also thinks its an easy bug to fix? Or you just dont have the time to do another re-write of something because you have to roll a release out...  Ye, I've been there, push-the-heck-back and get the product right! And delete this comment. :`x

Comment 75

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

Updated

7 years ago
Keywords: testcase

Comment 76

7 years ago
Hi guys.

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

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

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

Cheers,

Ed

Comment 77

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

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

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

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

Then you'll find out the time is changed in the page, but hidden the scroll bar why the object need be repainted?

Comment 78

7 years ago
(In reply to comment #74)
> Has to be said, viewing how this bug has been handled is a real eye opener. Its
> a complete blocker and an issue that only FF suffers from that has, and will
> hold back many web development projects, including the one I am working on...
> and here I am reading this log... 9 years guys, ... wow, let me guess, the next
> guy that takes this one also thinks its an easy bug to fix? Or you just dont
> have the time to do another re-write of something because you have to roll a
> release out...  Ye, I've been there, push-the-heck-back and get the product
> right! And delete this comment. :`x

Agreed completely. What's going on with this bug? 9 years is a little epic don't ya think?!

Comment 79

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

Updated

7 years ago
Duplicate of this bug: 340465

Comment 82

7 years ago
(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.
Duplicate of this bug: 502753
Duplicate of this bug: 507715

Comment 86

7 years ago
@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

Comment 89

7 years ago
This can't be a priority right now?  Is this a veiled attempt at discouraging the use of flash in Firefox?  This bug is 9 years old! If it's not a priority now when exactly will it be a priority.

Pretty disappointing...

Comment 90

7 years ago
(In reply to comment #89)
> This can't be a priority right now?  Is this a veiled attempt at discouraging
> the use of flash in Firefox?  This bug is 9 years old! If it's not a priority
> now when exactly will it be a priority.
> 
> Pretty disappointing...

Well, HTML5 says this needs to be done. So, some day.

Comment 91

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

Comment 94

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

Updated

7 years ago
Duplicate of this bug: 429428
Duplicate of this bug: 590891
(Assignee)

Comment 99

7 years ago
I'm planning to work on this for after Gecko 2.0.0.
Assignee: nobody → joshmoz

Comment 100

7 years ago
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.
Duplicate of this bug: 488167
Blocks: 614825

Comment 103

6 years ago
Hoping this gets fixed some day. :(

Comment 104

6 years ago
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...
Duplicate of this bug: 647739

Comment 106

6 years ago
Our ERP system is getting resigned from Oracle 10g forms to New Web interface. The forms section still there at least 70% now, and it's intimately working inside applet. It is an internal application and used above 2000 users. 

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

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

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

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

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

So we are waiting to get this bug fixed so we still can continue with FF.

Comment 107

6 years ago
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: ? → ---
(Assignee)

Comment 109

6 years ago
I am getting close to beginning work on this.
(Assignee)

Comment 110

6 years ago
I am starting work on this today.

Updated

6 years ago
Duplicate of this bug: 658980

Comment 112

6 years ago
Go Josh! I hate to pile onto the heated priority debate, but it needs to be pointed out that Facebook has basically become unusable in FF now (see bug 643683). That's a whole lotta users to lose...

Comment 113

6 years ago
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!
(Assignee)

Updated

6 years ago
Depends on: 660721
Duplicate of this bug: 661950
(Assignee)

Comment 115

6 years ago
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.
(Assignee)

Comment 116

6 years ago
Created attachment 538440 [details] [diff] [review]
fix v0.4

Posting this as public backup of my work so far. This will only compile on Mac OS X. Things work pretty well with this patch, including all basic web content I tried (YouTube, Hulu, ESPN). Plugin instances that are display:none work well now too (see included mochitest). Reframing is not quite there yet (I haven't dealt with widget ownership properly yet). The bfcache stuff I mentioned before is taken care of, so npruntime is fully functional now.

Comment 117

6 years ago
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.
(Assignee)

Comment 118

6 years ago
Created attachment 539517 [details] [diff] [review]
fix v0.6

Adds support for Linux, and re-framing works now.
Attachment #538440 - Attachment is obsolete: true
Created attachment 539607 [details] [diff] [review]
fix v0.6 + windows build fix

This version of the patch builds successfully on Windows too.
Attachment #539517 - Attachment is obsolete: true
(Assignee)

Comment 120

6 years ago
Created attachment 539735 [details] [diff] [review]
fix v0.7

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

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

One I have all tests passing I still need to go through this patch and address some comments I left for myself and generally clean things up before I request review from others. I'm pushing this patch to the holly branch tonight so that people can play with the resulting builds.
Attachment #539607 - Attachment is obsolete: true

Updated

6 years ago
Depends on: 665996

Updated

6 years ago
Duplicate of this bug: 665975
Duplicate of this bug: 666366
(Assignee)

Comment 123

6 years ago
Created attachment 541965 [details] [diff] [review]
fix v0.75

This has a crash fix, a memory leak fix, test fixes, and it supports destroying an instance by removing a DOM node from its document and leaving it unattached until control returns to the runloop. It regresses windowed plugins on Windows and Linux (they don't work at all). Windowless plugins should work just fine on Windows and Linux.
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.
(Assignee)

Comment 128

6 years ago
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.
(Assignee)

Comment 129

6 years ago
(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.

Comment 132

6 years ago
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
(Assignee)

Comment 133

6 years ago
Created attachment 542143 [details] [diff] [review]
fix v0.75 updated to latest m-c
Attachment #541965 - Attachment is obsolete: true
Blocks: 667213

Updated

6 years ago
Duplicate of this bug: 667213
(Assignee)

Comment 135

6 years ago
Created attachment 545206 [details] [diff] [review]
fix v0.76

Fixes a memory leak, updates to latest m-c.
Attachment #542143 - Attachment is obsolete: true
(Assignee)

Comment 136

6 years ago
Created attachment 545832 [details] [diff] [review]
fix v0.8

Makes windowed plugins work on Linux and Windows. Fixes a number of test failures.
Attachment #545206 - Attachment is obsolete: true
(Assignee)

Comment 137

6 years ago
Created attachment 547669 [details] [diff] [review]
fix v0.8, updated to current m-c
Attachment #545832 - Attachment is obsolete: true

Updated

6 years ago
Depends on: 674223

Updated

6 years ago
Depends on: 674224

Updated

6 years ago
Depends on: 675005
Duplicate of this bug: 670729

Updated

6 years ago
Duplicate of this bug: 675557

Comment 140

6 years ago
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.
(Assignee)

Comment 141

6 years ago
Created attachment 551786 [details] [diff] [review]
fix v0.8, updated to current m-c
Attachment #547669 - Attachment is obsolete: true
(Assignee)

Comment 142

6 years ago
Created attachment 554544 [details] [diff] [review]
fix v0.8, updated to current m-c
Attachment #551786 - Attachment is obsolete: true
(Assignee)

Comment 143

6 years ago
Created attachment 554760 [details] [diff] [review]
fix v0.82

Includes a fix for some test failures. Still have to resolve more test failures.
Attachment #554544 - Attachment is obsolete: true
(Assignee)

Comment 144

6 years ago
Created attachment 554998 [details] [diff] [review]
fix v1.0
Attachment #554760 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #554998 - Attachment description: fix v1.1 → fix v1.0
(Assignee)

Comment 145

6 years ago
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?
Attachment #554998 - Flags: review?(cbiesinger)
(Assignee)

Comment 147

6 years ago
(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.
(Assignee)

Comment 148

6 years ago
(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 :-).
(Assignee)

Comment 150

6 years ago
(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.
(Assignee)

Comment 151

6 years ago
Created attachment 555298 [details] [diff] [review]
fix v1.1

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

Comment 156

6 years ago
(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.
(Assignee)

Comment 157

6 years ago
(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.
(Assignee)

Comment 158

6 years ago
Created attachment 555626 [details] [diff] [review]
fix v1.2

More fixes, still have a couple of comments to address.
Attachment #555298 - Attachment is obsolete: true
Attachment #555298 - Flags: review?(roc)
(Assignee)

Comment 159

6 years ago
Created attachment 555832 [details] [diff] [review]
widget fix v1.3
Attachment #555626 - Attachment is obsolete: true
(Assignee)

Comment 160

6 years ago
Created attachment 555834 [details] [diff] [review]
other fix v1.3
(Assignee)

Updated

6 years ago
Attachment #555832 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #555834 - Flags: review?(roc)
(Assignee)

Comment 161

6 years ago
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-
Duplicate of this bug: 647943
(Assignee)

Comment 166

6 years ago
Created attachment 556221 [details] [diff] [review]
widget fix v1.4
Attachment #555832 - Attachment is obsolete: true
Attachment #556221 - Flags: review?(karlt)
(Assignee)

Comment 167

6 years ago
Created attachment 556404 [details] [diff] [review]
other fix v1.4
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.
(Assignee)

Comment 172

6 years ago
Created attachment 556468 [details] [diff] [review]
widget fix v1.5
Attachment #556221 - Attachment is obsolete: true
(Assignee)

Comment 173

6 years ago
Created attachment 556469 [details] [diff] [review]
other fix v1.5
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+
(Assignee)

Comment 175

6 years ago
Created attachment 556476 [details] [diff] [review]
other fix v1.6
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."
(Assignee)

Comment 177

6 years ago
(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.
(Assignee)

Comment 178

6 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/ab1cbc8a9d51
http://hg.mozilla.org/mozilla-central/rev/33031c875984
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Target Milestone: --- → mozilla9
(Assignee)

Updated

6 years ago
status-firefox9: --- → fixed

Updated

6 years ago
Depends on: 683007

Comment 179

6 years ago
Josh Aas,

Thank You!!!

Comment 180

6 years ago
some Flash content is no longer loaded after this checkin.

http://forums.mozillazine.org/viewtopic.php?p=11191871#p11191871
Blocks: 682951

Comment 181

6 years ago
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 :-)

Updated

6 years ago
No longer blocks: 682951
Depends on: 682951

Updated

6 years ago
Depends on: 683059

Updated

6 years ago
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.
(Assignee)

Comment 183

6 years ago
Agreed, backed out.

http://hg.mozilla.org/mozilla-central/rev/d5a0f6ad8bbd
http://hg.mozilla.org/mozilla-central/rev/1a4a5fcf0a76
http://hg.mozilla.org/mozilla-central/rev/a6e7760f2f36
http://hg.mozilla.org/mozilla-central/rev/e6591ea9b27b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 184

6 years ago
Created attachment 557076 [details] [diff] [review]
other fix v1.7

Updated to current trunk, no further fixes.
Attachment #556476 - Attachment is obsolete: true

Comment 185

6 years ago
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.

Updated

6 years ago
status-firefox9: fixed → ---
(Assignee)

Comment 187

6 years ago
Created attachment 558456 [details] [diff] [review]
other fix v1.8

Mainly improvements to code paths leading to NPP_SetWindow.
Attachment #557076 - Attachment is obsolete: true

Comment 188

6 years ago
1. What kinds of improvements were made? Are you referring to better code organizations, or is there actually any optimization occurring at this stage?
2. How did "improving code paths" lead to a resolution of bug 674224, or was that a side effect?

Thanks!

Comment 189

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

Comment 190

6 years ago
(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.
Blocks: 684618
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
(Assignee)

Comment 192

6 years ago
Created attachment 562159 [details] [diff] [review]
widget fix v1.6

Updated to current trunk.
Attachment #556468 - Attachment is obsolete: true
(Assignee)

Comment 193

6 years ago
Created attachment 562160 [details] [diff] [review]
other fix v1.9

Updated to current trunk with a few minor changes.
Attachment #558456 - Attachment is obsolete: true
(Assignee)

Comment 194

6 years ago
Created attachment 562343 [details] [diff] [review]
other fix v2.0

Nice catch in comment 191, Chris. Thanks. This patch has a fix with a test.
Attachment #562160 - Attachment is obsolete: true
(Assignee)

Comment 195

6 years ago
Created attachment 562982 [details] [diff] [review]
other fix v2.1

This includes a number of fixes, including a fix for Google Earth. Problem is, now Netflix doesn't work on Mac OS X.
Attachment #562343 - Attachment is obsolete: true

Updated

6 years ago
Target Milestone: mozilla9 → mozilla10
(Assignee)

Comment 196

6 years ago
Created attachment 565004 [details] [diff] [review]
widget fix v1.7

Updated to current trunk.
Attachment #562159 - Attachment is obsolete: true
(Assignee)

Comment 197

6 years ago
Created attachment 565011 [details] [diff] [review]
other fix v2.2

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

1) "test_zero_opacity.html" fails on Linux due to an assertion.
2) Netflix doesn't work on Mac OS X.
Attachment #562982 - Attachment is obsolete: true
Duplicate of this bug: 647943
(Assignee)

Comment 199

6 years ago
Created attachment 565860 [details] [diff] [review]
other fix v2.3

Fixes a number of test failures.
Attachment #565011 - Attachment is obsolete: true
Duplicate of this bug: 690525
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.
(Assignee)

Comment 205

6 years ago
Created attachment 568302 [details] [diff] [review]
widget fix v1.8

Updated to current m-c.
Attachment #565004 - Attachment is obsolete: true
(Assignee)

Comment 206

6 years ago
Created attachment 568303 [details] [diff] [review]
other fix v2.4

Updated to current m-c.
Attachment #565860 - Attachment is obsolete: true
(Assignee)

Comment 207

6 years ago
(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.
(Assignee)

Comment 208

6 years ago
Created attachment 568420 [details] [diff] [review]
other fix v2.5

Updated to current m-c.
Attachment #568303 - Attachment is obsolete: true

Comment 209

6 years ago
Created attachment 568600 [details]
An other way to get the bug

Get the bug just by changing the css positioning of the container.
(Assignee)

Comment 210

6 years ago
Created attachment 569455 [details] [diff] [review]
other fix v2.6
Attachment #568420 - Attachment is obsolete: true
(Assignee)

Comment 211

6 years ago
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.
(Assignee)

Comment 213

6 years ago
Thanks Karl! Very helpful, I have a fix which will be in my next patch.
(Assignee)

Comment 214

6 years ago
Created attachment 569797 [details] [diff] [review]
widget fix v1.9
Attachment #568302 - Attachment is obsolete: true
(Assignee)

Comment 215

6 years ago
Created attachment 569798 [details] [diff] [review]
plugin fix v2.7

All tests pass locally on Windows, Linux, and Mac OS X with this patch. Netflix is still broken on Windows and Mac OS X (didn't try on Linux).
Attachment #569455 - Attachment is obsolete: true
(Assignee)

Comment 216

6 years ago
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
No longer blocks: 684618
Depends on: 697651
Created attachment 570172 [details]
testcase showing reset on ancestor style change

Updating attachment 410850 [details] to use a url that is not 404.
Attachment #410850 - Attachment is obsolete: true

Comment 218

6 years ago
Changing Target Milestone to 11 based on comment 207
Target Milestone: mozilla10 → mozilla11
(Assignee)

Comment 219

6 years ago
Created attachment 571673 [details] [diff] [review]
plugin fix v2.8

Updated to current m-c. I'll be removing initialization for display:none plugins next, per the discussion in bug 697651.
Attachment #569798 - Attachment is obsolete: true
(Assignee)

Comment 220

6 years ago
Created attachment 571752 [details] [diff] [review]
plugin fix v2.9

Removes support for instantiating display:none instances. Also fixes a crash I've been seeing once in a while and finally tracked down.
Attachment #571673 - Attachment is obsolete: true
(Assignee)

Comment 221

6 years ago
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.
(Assignee)

Comment 223

5 years ago
Created attachment 574308 [details] [diff] [review]
widget fix v2.0

Updated to current m-c.
Attachment #569797 - Attachment is obsolete: true
(Assignee)

Comment 224

5 years ago
Created attachment 574310 [details] [diff] [review]
plugin fix 3.0

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

Thanks for the report Johnny! This patch might fix your crash, I'll look more closely at it today though.
Attachment #571752 - Attachment is obsolete: true
(Assignee)

Comment 225

5 years ago
Created attachment 574784 [details] [diff] [review]
plugin fix 3.1

Fixes test failures. The only reliably failing test remaining is "test_pluginstream_err.html" on Windows and Linux.
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, 
...
(Assignee)

Comment 227

5 years ago
(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.
Part 1:
https://hg.mozilla.org/mozilla-central/rev/ec384df0ce77
(Assignee)

Comment 230

5 years ago
Created attachment 577622 [details] [diff] [review]
plugin fix v3.2

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

The only known problem with this patch is Pandora on Windows. All tests pass.
Attachment #574784 - Attachment is obsolete: true

Comment 231

5 years ago
> The only known problem with this patch is Pandora on Windows. All tests pass.

Sounds like we're missing a test ;)
Created attachment 577821 [details]
Log of pandora loading w/o this patch.
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?
(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.
Blocks: 706669
(Assignee)

Comment 235

5 years ago
Created attachment 578336 [details] [diff] [review]
plugin fix v3.3

Updated to current trunk.
Attachment #577622 - Attachment is obsolete: true
(Assignee)

Comment 236

5 years ago
Created attachment 578409 [details] [diff] [review]
plugin fix v3.4

Last patch I uploaded was missing some test files.

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

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

That needs to be fixed and a test for it added in addition to a fix for the Pandora issue on Windows.
Attachment #578336 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 707052
(Assignee)

Comment 237

5 years ago
Created attachment 578479 [details] [diff] [review]
plugin fix v3.5

Fixes the regression documented in bug 707052. There is an automated test in that bug which works locally but it's hard to do in a reliable way, one that we'd want to check in. I'll keep thinking about it.
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.

Updated

5 years ago
Target Milestone: mozilla11 → ---

Updated

5 years ago
Depends on: 713552
(Assignee)

Comment 240

5 years ago
Created attachment 584353 [details] [diff] [review]
plugin fix v3.6

Updated to current trunk. Includes a fix for an assertion on Mac OS X re: layer-based painting. Everything passes on try server but we still need a fix for Pandora on Windows once we figure out what is going on there.
Attachment #578479 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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).

Comment 244

5 years ago
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.
Blocks: 501485
(Assignee)

Comment 245

5 years ago
Created attachment 587250 [details] [diff] [review]
plugin fix v3.7

Updated to current trunk.

I don't have access to a dev machine with Windows right now and I can't reproduce the Pandora using Benjamin's reduced test case on Mac OS X. I'll try reproducing on Linux next.
Attachment #584353 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #587250 - Attachment is patch: true
(Assignee)

Comment 246

5 years ago
I can't reproduce the Pandora bug using Benjamin's test case on Linux either.
Created attachment 587367 [details] [diff] [review]
Mochitest to ensure that we don't deliver the data="" data twice, rev. 1

This is a mochitest-style test for the "pandora issue" that should work on any platform. It tests that we only receive the <object data=""> data once, and it currently passes on m-c. So I'll just land it now after review.
Attachment #587367 - Flags: review?(joshmoz)
(Assignee)

Updated

5 years ago
Attachment #587367 - Flags: review?(joshmoz) → review+
http://hg.mozilla.org/mozilla-central/rev/baa88d873a77 (the multiple-stream test) landed yesterday.
https://hg.mozilla.org/mozilla-central/rev/baa88d873a77
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Comment 250

5 years ago
This isn't fixed, that was just a test that already passes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla12 → ---
(Assignee)

Updated

5 years ago
No longer blocks: 89077
(Assignee)

Comment 251

5 years ago
Created attachment 588161 [details] [diff] [review]
plugin fix v3.8

Updated to current trunk and contains a stream fix for plugins that don't have object frames. This patch still has "the Pandora problem."
Attachment #587250 - Attachment is obsolete: true

Comment 252

5 years ago
Is there a test case for the XStandard plugin?
http://www.xstandard.com/

Can one be created? Is it just a HTML file? Can both v2 and v3 be tested, on both windows and mac?

Comment 253

5 years ago
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
(Assignee)

Comment 254

5 years ago
Created attachment 588307 [details] [diff] [review]
plugin fix v3.9

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

I'm going to keep testing but right now I'm not aware of any more problems with this patch. Requesting review again as I've made significant changes since the last time we did a review.
Attachment #577821 - Attachment is obsolete: true
Attachment #577822 - Attachment is obsolete: true
Attachment #588161 - Attachment is obsolete: true
Attachment #588307 - Flags: review?
(Assignee)

Updated

5 years ago
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.

Comment 257

5 years ago
(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.

Updated

5 years ago
Blocks: 718287

Comment 258

5 years ago
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.

Comment 260

5 years ago
Confirming on Seven x64 here, bug fixed on try build above.

Comment 261

5 years ago
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.
(Assignee)

Comment 262

5 years ago
Created attachment 589928 [details] [diff] [review]
plugin fix v4.0

Updated to current trunk.
Attachment #588307 - Attachment is obsolete: true

Comment 263

5 years ago
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.
(Assignee)

Comment 265

5 years ago
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.
(Assignee)

Comment 266

5 years ago
Created attachment 590969 [details] [diff] [review]
plugin fix v4.1

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

I don't know of any remaining issues with this patch, but I'll keep testing with this last change.
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+
(Assignee)

Comment 268

5 years ago
Created attachment 592143 [details] [diff] [review]
plugin fix v4.2

Updated to current trunk.
Attachment #590969 - Attachment is obsolete: true
(Assignee)

Comment 269

5 years ago
pushed plugin fix v4.2 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/15b00ab7f22d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Target Milestone: --- → mozilla13
Depends on: 723162

Updated

5 years ago
Depends on: 723154
Depends on: 723164

Updated

5 years ago
Depends on: 723291
Depends on: 723379

Updated

5 years ago
Depends on: 519752

Updated

5 years ago
Depends on: 285982

Updated

5 years ago
Depends on: 723473

Updated

5 years ago
Depends on: 723476

Updated

5 years ago
Depends on: 723503

Comment 270

5 years ago
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.

Updated

5 years ago
Depends on: 723520

Updated

5 years ago
Depends on: 723523

Updated

5 years ago
Depends on: 723133

Updated

5 years ago
Depends on: 723529
Depends on: 723558
(Assignee)

Updated

5 years ago
No longer depends on: 723503

Updated

5 years ago
Depends on: 723856

Updated

5 years ago
No longer depends on: 723856

Updated

5 years ago
Depends on: 724248

Updated

5 years ago
Depends on: 724250

Updated

5 years ago
Depends on: 724532
Depends on: 724651

Updated

5 years ago
Depends on: 724717

Updated

5 years ago
Depends on: 724812

Updated

5 years ago
Depends on: 725279
Depends on: 723190
(Assignee)

Updated

5 years ago
Depends on: 723217
Depends on: 725917

Updated

5 years ago
Blocks: 633985
Depends on: 727211
Depends on: 726734
Depends on: 728672
Depends on: 726278
Depends on: 737433

Updated

5 years ago
Depends on: 724781

Updated

5 years ago
Depends on: 736998

Updated

5 years ago
Depends on: 753251

Updated

5 years ago
Duplicate of this bug: 752827

Comment 273

5 years ago
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

Updated

5 years ago
Depends on: 776451

Updated

5 years ago
Depends on: 784131

Updated

5 years ago
Depends on: 788900

Updated

4 years ago
Depends on: 825188
Duplicate of this bug: 335583

Updated

4 years ago
Depends on: 767673

Updated

4 years ago
Depends on: 865609

Updated

4 years ago
Depends on: 874027

Comment 276

4 years ago
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.

Comment 278

4 years ago
ah indeed this does look the same as bug 889614, apologies on not finding that one

Comment 279

2 years ago
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
You need to log in before you can comment on or make changes to this bug.