Closed Bug 179822 Opened 17 years ago Closed 17 years ago
Flash4 / Flash5 / Shockwave and other plugins crash in recent trunk builds compiled with MOZ
Using BuildID 2002111208 on Win2KSP3 Open up a jpg in the browser window Create a new tab and then try to go to the above URL Actual Results Browser crashes Talkback ID TB13853056K Expected Results Browser doesn't crash This doesn't happen in BuildID 2002110808 so adding regression keyword, maybe candidate for zt4newcrash
0xffff0565 npswf32.dll + 0xbb7a (0x038bbb7a) npswf32.dll + 0xae46 (0x038bae46) USER32.dll + 0x46fc (0x77e146fc) USER32.dll + 0x74ad (0x77e174ad) ntdll.dll + 0x202ff (0x77fa02ff) nsView::SetDimensions [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 530] nsViewManager::ResizeView [c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2565] nsLineLayout::ReflowFrame [c:/builds/seamonkey/mozilla/layout/html/base/src/nsLineLayout.cpp, line 1268] nsInlineFrame::ReflowInlineFrame [c:/builds/seamonkey/mozilla/layout/html/base/src/nsInlineFrame.cpp, line 719] nsInlineFrame::ReflowFrames [c:/builds/seamonkey/mozilla/layout/html/base/src/nsInlineFrame.cpp, line 526] nsInlineFrame::Reflow [c:/builds/seamonkey/mozilla/layout/html/base/src/nsInlineFrame.cpp, line 439] nsLineLayout::ReflowFrame [c:/builds/seamonkey/mozilla/layout/html/base/src/nsLineLayout.cpp, line 1048] nsBlockFrame::ReflowInlineFrame [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3902] nsBlockFrame::DoReflowInlineFrames [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3730] nsBlockFrame::DoReflowInlineFramesAuto [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3634] nsBlockFrame::ReflowInlineFrames [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3579] nsBlockFrame::ReflowLine [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2653] nsBlockFrame::ReflowDirtyLines [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2297] nsBlockFrame::Reflow [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 944] nsBlockReflowContext::ReflowBlock [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockReflowContext.cpp, line 537] nsBlockFrame::ReflowBlockFrame [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3337] nsBlockFrame::ReflowLine [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2519] nsBlockFrame::ReflowDirtyLines [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2297] nsBlockFrame::Reflow [c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 944] nsContainerFrame::ReflowChild [c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 949] CanvasFrame::Reflow [c:/builds/seamonkey/mozilla/layout/html/base/src/nsHTMLFrame.cpp, line 570] nsBoxToBlockAdaptor::Reflow [c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBoxToBlockAdaptor.cpp, line 929] nsBoxToBlockAdaptor::DoLayout [c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBoxToBlockAdaptor.cpp, line 670] nsBox::Layout [c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBox.cpp, line 1066] nsScrollBoxFrame::DoLayout [c:/builds/seamonkey/mozilla/layout/xul/base/src/nsScrollBoxFrame.cpp, line 361] nsBox::Layout [c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBox.cpp, line 1066] nsBoxFrame::Reflow [c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 900] nsContainerFrame::ReflowChild [c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 949] ViewportFrame::Reflow [c:/builds/seamonkey/mozilla/layout/html/base/src/nsViewportFrame.cpp, line 579] IncrementalReflow::Dispatch [c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 896] PresShell::ProcessReflowCommands [c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6492] ReflowEvent::HandleEvent [c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6337] PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 645] PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 578] _md_EventReceiverProc [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 1336] nsAppShellService::Run [c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 472] main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1557] main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1905] WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1925] WinMainCRTStartup() KERNEL32.DLL + 0x1ca90 (0x77e9ca90)
Ian: I bet you use flash5. Can you please upgrade to flash6 ? see also bug 179743 and bug 179713.
Assignee: other → beppe
Component: Layout → Plug-ins
QA Contact: ian → shrir
Beppe : Is there a way to avoid this plugin crash on the mozilla side ? We will get many dupes in the future :-(
Not really, if Flash had a sniffer that forced a dialog asking people to upgrade is about the only thing I can think of at the moment. And, yes there will be tons of dupes. Let me check and see if there is something we can do with Macromedia.
This is a regression, probably recent. Flash 5 didn't used to crash. Can someone who is crashing with Flash 5 please pull daily builds to isolate what checkin caused this. Thanks! Also, this sounds like a dup of bug 170453 which has now come back.
peterl : from bug 179713 : The last version I was using was from end of october, IIRC, and didn't have such a crash.
*** Bug 179713 has been marked as a duplicate of this bug. ***
note, I'm unable to update my version of flash because I crash when I go to http://www.macromedia.com/software/flashplayer/special/beta/
I was able to update flash using IE. has something in plugin land changed that makes it so flash 5.x crashes now?
note: You can rename the npswf.dll (to "npswf.DL_") to disable flash and you should be able to upgrade with mozilla.
> This is a regression, probably recent. Flash 5 didn't used to crash. > Can someone who is crashing with Flash 5 please pull daily builds to isolate > what checkin caused this. Thanks! Peter, From my comments in bug 179713 (additionaly comment #2): > I've tested old version of the nightlies: > 20/10 works ok > 30/10 works ok > 8/11 works ok > 11/11 crashes > So this is a recent regression. > I could not test 9/11 and 10/11, as I've found no Windows nightly builds of the > 1.2 on the ftp server (the only 10/11 I've found is the 1.0 mozilla trunk). bug 179713 is a duplicate of 179822, due to my pre-cognition skills :)
There was no crashes in BuildID 2002110808 but there was in BuildID 2002111104 on WinXP. Unfortunately there were no nightly builds between those builds so I can't narrow down the crash any more than that. The only bugs to touch files mentioned in the stacktrace are bug 56088, bug 170011, bug 176915 and bug 143815. I suspect there will be a high number of dupes as not everyone will have upgraded to flash 6 if that is the cause.
Why would this be a regression? If the site upgraded their plug-in files to use scripting.
>Why would this be a regression? If the site upgraded their plug-in files to use scripting. I can tell that at least the site cplus.fr has not upgraded their video/flash plugin (it's one on the site i mentionned in 179713). So for me this is a regression. But I cannot test any more as I have flash 6 now.
on second thought, Peter -- how difficult would it be to sniff the version of flash that is installed and throw-up a dialog that informs the user to upgrade to a newer verison.
*** Bug 179924 has been marked as a duplicate of this bug. ***
Summary: faceparty.com - browser crash → Flash4 / Flash5 crash Mozilla in recent trunk builds
Click on this link to install Flash 5 which came with Netscape 6.0: ftp://ftp.netscape.com/pub/netscape6/english/6.0/windows/win32/xpi/flash.xpi (you may need to restart to use it) Bonsai query: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=11%2F08%2F2002+8%3A00&maxdate=11%2F11%2F2002+8%3A00&cvsroot=%2Fcvsroot
I think this is a regression from bug 104934. When I rebuilt widget/src with this patch to turn off MOZ_UNICODE, Flash 5 no longer crashes. I'm not sure what we can do here besides black list and stongly urge users to upgrade if Flash 5 can't deal with Mozilla using windows unicode functions. You can get the same scriptable Flash 6 that shipped with Netscape 7 that does not crash without going having to go to Macromedia by click on this link: ftp://ftp.netscape.com/pub/netscape7/english/7.0/windows/win32/ewc9e/flash.xpi
*** Bug 180071 has been marked as a duplicate of this bug. ***
This also seems to be affecting Phoenix (bug 179954) and Chimera (bug 180054) would this UNICODE patch caused the problem in those browsers too?
*** Bug 179954 has been marked as a duplicate of this bug. ***
>This also seems to be affecting Phoenix (bug 179954) and Chimera (bug 180054) >would this UNICODE patch caused the problem in those browsers too? No Unicode patch is only for Windows platforms. I'll take a look at this bug. taking.
Assignee: beppe → yokoyama
No. Unicode patch is only for Windows platforms. It doesn't cause any problems for other platform.s
peter: I can't reproduce this bug. I installed ftp://ftp.netscape.com/pub/netscape6/english/6.0/windows/win32/xpi/flash.xpi and went to macromedia site. I verified that i have indeed flash by <Help/About plugin>. It shows... File name: npswf32.dll Shockwave Flash 5.0 r30 Ian/ylong: can you try the older unicode enabled nightly build; say ftp://ftp.mozilla.org/pub/mozilla/nightly/2002-11-08-08-unicode/ Other instabilities in Comment #20 may be caused by others.
I tried the unicode build of 2002110808 on WinXPSP1 and this crashes too, flash plugin version is 5.0r41 Phoenix would be affected in it's windows version, but the Chimera problem is another bug so that can be ignored.
Ian: can you try upgrading to Flash 6 and see if that does indeed prevent the crashes on the other builds?
Yes upgrading to flash 6 on earlier unicode enabled builds does fix the problem, so the problem is with flash 4/5 in unicode enabled builds.
*** Bug 180445 has been marked as a duplicate of this bug. ***
*** Bug 180538 has been marked as a duplicate of this bug. ***
*** Bug 180698 has been marked as a duplicate of this bug. ***
*** Bug 171178 has been marked as a duplicate of this bug. ***
It looks like the change from SetWindowLong() to SetWindowLongW() causes to crash.
hm...plugin code uses the |SubclassWindow| function here: http://lxr.mozilla.org/mozilla/source/modules/plugin/base/src/nsPluginNativeWindowWin.cpp#355 ...and it looks like nsWindow.cpp uses |nsToolkit::mSetWindowLong| when MOZ_UNICODE is defined. Maybe plugin code should do the same?
This is not a function, this is a macros defined in <windowsx.h> to map to SetWindowLong: #define SubclassWindow(hwnd, lpfn) \ ((WNDPROC)SetWindowLong((hwnd), GWL_WNDPROC,(LPARAM)(WNDPROC)(lpfn)))
I observed a bizzar behavior before when SetWindowLong() and GetWindowLong() doesn't match. One calls W-API; but the other calls A-API. (ie. call SetWindowLongW() and GetWindowLong() ) This may be the similar case. I replaced the SubclassWindow() macro with SetWindowLongW() and the other S/GetWindowLong() calls in nsPluginNativeWindowWin.cpp to S/GetWindowLongW() However, the plugin still crashes.
Roy, can you please try to make an early return from |SubclassAndAssociateWindow| and see if it still crashes. No Set/GetWindowLong methods will be called in this case, and we will know if we need to tune the |nsPluginNativeWindowWin| code or not.
av: no, it still crashes. :( Early return from SubclassAndAssociateWindow() didn't help. Note: I also did the same for UndoSubclassAndAssociateWindow(), just in case. But didnt' help either.
*** Bug 180923 has been marked as a duplicate of this bug. ***
Looks like our code in nsPluginNativeWindowWin.cpp is not relevant at all, the crash happens earlier, before the very first call to SetWindow. I think this is inaccuracy in using A and W versions of Windows functions somewhere in the widget code. By the way, those functions are used in a couple of more places in the application. Roy, your changes to convert Mozilla to unicode -- do they cover all necessary places?
This bug should either be fixed for 1.2 or mentioned in the release notes
>This bug should either be fixed for 1.2 ?? Unicode never turned on for 1.2. If moz is crashing in 1.2; then it's not caused by Unicode change. Please remove "mozilla1.2" keyword from this bug. >do they cover all necessary places? Yes, as much as I can see. However, I may have missed other places.
Just to update. Calling SetWindowLong() instead of nsToolkit::mSetWindowLong() stops the crash; but not recommended. http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#4924
Just tested - 1.2 is not affected. Thx.
peter: do we have document for the flash 6? I'd like to see what has changed from 4/5 to 6.
Roy, should not we also define MSVC var for unicode on Windows projects? Something like: DEFINES += -DMOZ_UNICODE -DUNICODE
>should not we also define MSVC var for unicode on Windows projects? No, if we do, then we need to change lots of codes. All the APIs will expect WCHAR * instead of char *. As a matter of fact, I tried to do that initially; but I gave up on it. It just tooo much to do. Plus we need to support Win9x base systems. If we define -DUNICODE, then we have to link to MSLU (Microsoft Layer for Unicode).
OK, sounds reasonable. I also noticed that we don't do A and W versions for SetProp and GetProp, while those functions are used to store winproc. Can this be related?
>SetProp and GetProp I tried the W version for those; but it didnt' fix the crash. However, I may need to change to W version in later days.
This is blocking a Phoenix release -- any eta?
I looked everywhere for inconsistancy between A and W versions of Windows functions. But the flash still crashes. I drilled down to the exact Windows API causing to crash. It's ::SetWindowPos() !! http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#2074 Can get some help from Macromedia? Do they support this version of Flash for an Unicode application?
You may all want to know that the same behavior is exhibited in Shockwave as well. Please see bug 180934, not duping that so Macromedia folks are aware of the Shockwave side of things.
I think unicode support was added in Flash 6. I imagine the 'fix' is to upgrade. cc:ing emillard for insight.
The other possibility is that we can dynamically change to non-Unicode application when we detect the flash4/5 is loaded. We are currently doing this at startup-runtime for supporting the legacy Windows 9.x OSs.
*** Bug 181027 has been marked as a duplicate of this bug. ***
Flash 6 uses Unicode for Flash 6 SWF's. It uses ShiftJIS and Windows Latin1 for high ASCII on version 5 and earlier SWF's. There is no Korean or Chinese support prior to Flash 6.
Ed: I'm sorry; but I failed to see the relationship between this bug and unicode codepoints. This bug is about the flash plugin crashing in Unicode application; not by the unicode codepoints. ::SetWindowPos() API doesn't deal with unicode codepoints at all. Is the flash 4/5 supported in an unicode application (ie. an app compiled with -DUNICODE in Win32)? What do you mean by "Flash 6 uses Unicode for Flash 6 SWF's"?
Here's a single-line file that will reproduce this error (at least the one I'm getting, which I'm almost sure is the one that this bug describes). Note that the Flash file doesn't even have to EXIST in order for the browser to crash. Also, in case you need more data, here are my talkback IDs of my crashes: TB14194245Y TB14194182K TB14194166W TB14194149H And yes, I have Flash 4. Of course, that's the default, if you have a fresh O/S install and you just download Moz and don't ever get a new plugin. HTH! -M
*** Bug 181258 has been marked as a duplicate of this bug. ***
*** Bug 181271 has been marked as a duplicate of this bug. ***
*** Bug 181311 has been marked as a duplicate of this bug. ***
Roy, this is true that |SetWindowPos| does not have A and W versions but it does not only changes window placement but also sends a notification message. And SendMessage which is apparently used by the system has a unicode version. It crashes namely on sending a message. If you supress this by adding a flag SWP_NOSENDCHANGING to |SetWindowPos| you will be able to get through this call, plugin will still crash but now it will be caught by our exception handling code. The following code is equivalent to |SetWindowPos| we have now but separates SendMessage so you can see how it crashes there: VERIFY(::SetWindowPos(mWnd, NULL, 0, 0, aWidth, GetHeight(aHeight), flags | SWP_NOSENDCHANGING)); WINDOWPOS wp; wp.hwnd = mWnd; wp.hwndInsertAfter = NULL; wp.x = 0; wp.y = 0; wp.cx = aWidth; wp.cy = GetHeight(aHeight); wp.flags = flags; SendMessage(mWnd, WM_WINDOWPOSCHANGED, NULL, (LPARAM)&wp); I tried to use both A and W versions of |SendMessage| here but both crash. Also, I don't know why we haven't mentioned this before... Flash _subclasses_ our window. Can the problem be because it uses |SetWindowLongA|? I tried to play with our Basic plugin sample which also subclasses the plugin window but could not model this crash. Looks like I am out of ideas now.
*** Bug 181362 has been marked as a duplicate of this bug. ***
beppe: Can I get some help from Macromedia team? I am also out of ideas.
I filed a bug for you in the Macromedia database, the Windows player isn't my department, only Linux. I wouldn't count on immediate assistance since we are in a release push for the next Flash 6 release and it will probably take precedence.
roy: re: comment #53, how can we dynamically switch off unicode for [some] plugins?
*** Bug 181939 has been marked as a duplicate of this bug. ***
Peter: We don't have the machanism to change to non-Unicode dynamically; but we can make the application to non-Unicode by having another condition to _ if (nsToolkit::mIsNT) _ http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsToolkit.cpp#593 Current implementation is to call W version for NT base platforms. We may need to restart the app when we detect the new plugins are installed.
Peter: Can you try this patch? I got the flash plugins to display; but I am getting bunch of assertions at \layout\html\base\src\nsFrame.cpp  NS_ASSERTION(value != 0, "frame state bit was set but frame has no view");
Never mind, peter. It still crashes..... :(
ok, thanks to Peter for giving me an idea or two to try this beast under control. I think I got it to make it work and still we have the unicode functionalities in tact. The idea is to create an A version of Widget conditionally by passing a new PRBool parameter from nsObjectFrame to Widget. My source is messy; so after the verification, I will post a patch today.
We didn't need to create the non-Unicode Widget in order to fix this crash. The patch provides a mechanism to call between ::SetWindowLongW() and ::SetWindowLongA() inside nsWindow::SubclassWindow() nsObjectFrame::CreateWidget() now accepts unicode flag. Peter: can you try my patch for flash and shockwave plugins? Kin: I need your feed back on the changes in nsWidget module, particulary in nsWindow::SubclassWindow(). It appears that we call SubclassWinodw(PR_TRUE) when we create Widget object and call SubclassWinodw(PR_FALSE) when we destroy it. I want to avoid the situattion where we call SetWindowLongA() on creation; but call SetWindowLongW() on destroy. I didn't see any mis-match while debugging it. Can you think of any situations?
Attachment #107483 - Attachment is obsolete: true
Roy, that patch looks pretty good. I'm no longer crashing with Flash 5. Just wondering though if you could use nsWidgetInitData to hold the unicode flag and init with instead of adding extra arguments to nsObjectFrame.h and nsIView.h methods. I think nsObjectFrame should always create a non-unicode window.
Maybe to introduce a flag setter as deep as possible and set it from nsObjectFrame code? This may be not possible though, as subclassing already happened during |nsObjectFrame::CreateWidget| call. But if can reduce the chain of passing this flag over, this would look better.
>I think nsObjectFrame should always create a non-unicode window. I am not very familiar with nsobjectFrame module. What would be the impact of always making non-unicode window in nsObjectFrame?
*** Bug 182312 has been marked as a duplicate of this bug. ***
peter: It's very straight forward change and it should work; but for some reason mozilla freezes at the startup. I clobber_all'ed just to be sure; but no cigar. Do you see this kind of behavior before?
Attachment #107529 - Attachment is obsolete: true
Silly me, I forgot to check for nsnull for nsWidgetInitData *aInitData. I'll post a new patch in a minute.
Peter: can you review?
Attachment #107631 - Attachment is obsolete: true
Roy, in |nsView::CreateWidget| there is some meaning of |aWidgetInitData| being null. In such a case it will try to assign a parent to the appropriate field in the struct. With this patch, are not we going to leave that code path aside? Can it break something?
I've asked the similar question to Peter thru private emails; and only plugin calles |nsObjectFrame::CreateWidget| which, in this case , is the caller of |nsView::CreateWidget| with |aWidgetInitData| being not null. Index: layout/html/base/src/nsObjectFrame.cpp - - result = view->CreateWidget(kWidgetCID); - + nsWidgetInitData initData; + initData.mUnicode = PR_FALSE; + result = view->CreateWidget(kWidgetCID, &initData); >Can it break something? It shouldn't. :)
That's what I mean. With this patch that field will never be tried to be assigned, as opposite to what it was before. (I was wrong, this is not parent but rather |nsWindgetInitData::mListenForResizes|.) I am not saying this is wrong, there are other conditions, so the field may not be assigned anyway. I just want to make sure we understand this. I am talking about |if| block here: nsView.cpp#769.
I think we need input from Peter about this. Peter?
*** Bug 182664 has been marked as a duplicate of this bug. ***
*** Bug 182775 has been marked as a duplicate of this bug. ***
Adding the blocking1.3a+ flag as this is wanted by firstname.lastname@example.org for Mozilla 1.3a scheduled to freeze on Dec 4.
*** Bug 182030 has been marked as a duplicate of this bug. ***
The |if| statement in nsView.cpp#769 won't set |mListenForResizes| but that field only seems to be used in certain toolkits anyway. I guess we can set it here to be safe. Otherwise, it seems like it does the same thing in terms of nsWidgetInitData fields and getting the same parent widget. One thing though, we need the same changes in nsPluginViewer.cpp's widget for full-page plugins.
peter: can you verify the case where we use full-page plugins? and review? I want to checkin this patch today. Thanks
Attachment #107639 - Attachment is obsolete: true
This version looks fine to me.
*** Bug 182843 has been marked as a duplicate of this bug. ***
I reported bug 182843 (Download Accelerator causes crash) which turns out to be duplicate of this one. Probably the only keyword in this summary that my bug has in common with this one is "crash". Perhaps the summary of this bug should include extra kewords to indicate that not just the Flash plugin is affected by this.
Comment on attachment 107929 [details] [diff] [review] add mListenForResizes = PR_TRUE; r=peterl
Attachment #107929 - Flags: review?(peterlubczynski) → review+
Comment on attachment 107929 [details] [diff] [review] add mListenForResizes = PR_TRUE; kin: I have changed struct of nsWidgetInitData. New member _mUnicode_ is added to allow the non-unicode plugins to subclass the widget. Please super review. Thanks
Attachment #107929 - Flags: superreview?(kin)
Summary: Flash4 / Flash5 crash Mozilla in recent trunk builds → Flash4 / Flash5 / Shockwave and other plugins crash in recent trunk builds compiled with MOZ_UNICODE
Comment on attachment 107929 [details] [diff] [review] add mListenForResizes = PR_TRUE; The patch looks fine to me, but I'm concerned about the |mListenForResizes| change in nsObjectFrame.cpp. As av pointed out above, this will impact certain tookits, in particular the gtk2 toolkit. Note that the code in nsView::CreateWidget() only sets this to true if the view creating the widget has a parent, and that parent has a different view manager than the view itself, which is usually the case when the view we are talking about is the root view for an iframe/frame. Are you sure we need to set this true for plugins? Do plugins create their own view managers? I'm not sure what the impact of having mListenForResizes be PR_TRUE in terms of notifications dispatched/received, etc, when using gtk2. + initData.mListenForResizes = PR_TRUE; Also, are you sure we need to set mListenForResizes to PR_TRUE in nsPluginViewer.cpp since it never triggered the code in nsView::CreateWidget() in the first place?
Would it make sense to mimic the logic in |nsView::CreateWidget| trying to get the parent and then do the same thing?
av, I was wondering that myself, though I think we may be able to get away with leaving it false for plugins. Plugins are the only thing that calls nsObjectFrame::CreateWidget() right? If as roc pointed out to me on IRC, an object frame of type "text/html" does trigger nsObjectFrame::CreateWidget(), then I think we may have to mimic the logic to make sure nothing regresses.
I'm not breaking in nsObjectFrame::CreateWidget when I have a testcase like: <object type="text/html" data="http://www.mozilla.org" width=400 height=400> </object> That method should only be called when we think we have a plugin. The nsFrameFrame creates the IFRAME widget. Maybe |mListenForResizes| should be left alone.
So we are going to remove + initData.mListenForResizes = PR_TRUE; from both nsObjectFrame.cpp and nsPluginViewer.cpp, right? or just to remove from nsObjectFrame.cpp ?
Comment on attachment 107929 [details] [diff] [review] add mListenForResizes = PR_TRUE; peterl, thanks for verifying that. I say remove them both, it shouldn't be a factor in the nsPluginInstance case since it bypasses the view code to create the widget. Do that and you can carry over the sr=kin to the new patch you post.
Attachment #107929 - Flags: superreview?(kin) → superreview+
Yes, initData.mListenForResizes = PR_TRUE; is NOT needed for plugins. It IS needed for the cases where Mozilla is rendering the OBJECT as a subdocument. [IIRC, this flag tells the widget whether OS-level widget resize events should notify the view manager of a size change.]
thanks, peter, av and kin.
Attachment #107929 - Attachment is obsolete: true
Comment on attachment 108107 [details] [diff] [review] removing mListenForResizes carry over reviews with permission /r=peterl /sr=kin
checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
looks good to me. I did not crash on XP with today's trunk 1204. Reporter, pls reverify that it fixes your crash. thx!!
Status: RESOLVED → VERIFIED
*** Bug 170453 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.