Closed Bug 179822 Opened 22 years ago Closed 22 years ago

Flash4 / Flash5 / Shockwave and other plugins crash in recent trunk builds compiled with MOZ_UNICODE

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.3alpha

People

(Reporter: iannbugzilla, Assigned: tetsuroy)

References

(
URL
)

Details

(Keywords: crash, regression, Whiteboard: px)

Attachments

(3 files, 5 obsolete files)

hack to turn off MOZ_UNICODE
2.14 KB, patch
Details | Diff | Splinter Review
single-line testcase
21 bytes, text/html
Details
removing mListenForResizes
5.35 KB, patch
tetsuroy
: review+
tetsuroy
: superreview+
Details | Diff | Splinter Review 
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)
Keywords: crash, regression, stackwanted
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.
Keywords: stackwanted
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
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
Status: NEW → ASSIGNED
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
Keywords: mozilla1.2, relnote
>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?
Whiteboard: px
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.
Blocks: 157673
*** 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"?
Target Milestone: --- → mozilla1.3alpha
Attached file single-line testcase — 
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
[2262]
       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. ***
Blocks: 180934
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.
Attached patch revised (obsolete) — — Splinter Review 
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 drivers@mozilla.org for
Mozilla 1.3a scheduled to freeze on Dec 4.
Flags: blocking1.3a+
*** 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.
Attached patch add mListenForResizes = PR_TRUE; (obsolete) — — Splinter Review 
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
Attachment #107929 - Flags: review?(peterlubczynski)
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
Attachment #108107 - Flags: superreview+
Attachment #108107 - Flags: review+
checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 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. ***
No longer blocks: 157673
Depends on: 180372
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: