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

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Ian Neal]

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

Comment 1

[Doron Rosenberg (IBM)]

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) 

Comment 2

[Matthias Versen [:Matti]]

Ian: I bet you use flash5. Can you please upgrade to flash6 ?

see also bug 179743 and bug 179713.

Comment 3

[Matthias Versen [:Matti]]

Beppe : 
Is there a way to avoid this plugin crash on the mozilla side ?
We will get many dupes in the future :-(

Comment 4

[rubydoo123]

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.

Comment 5

[Peter Lubczynski]

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.

Comment 6

[Matthias Versen [:Matti]]

peterl : from bug 179713 :
The last version I was using was from end of october, IIRC, and didn't have such
a crash.

Comment 7

[(not reading, please use [e-mail] instead)]

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

Comment 8

[(not reading, please use [e-mail] instead)]

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/

Comment 9

[(not reading, please use [e-mail] instead)]

I was able to update flash using IE.

has something in plugin land changed that makes it so flash 5.x crashes now?

Comment 10

[Matthias Versen [:Matti]]

note: You can rename the npswf.dll (to "npswf.DL_") to disable flash and you
should be able to upgrade with mozilla.

Comment 11

[oliv]

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

Comment 12

[Ian Neal]

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.

Comment 13

[rubydoo123]

Why would this be a regression? If the site upgraded their plug-in files to use
scripting. 

Comment 14

[oliv]

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

Comment 15

[rubydoo123]

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.

Comment 16

[Matthias Versen [:Matti]]

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

Comment 17

[Peter Lubczynski]

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

Comment 18

[Peter Lubczynski]

Attachment: hack to turn off MOZ_UNICODE

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

Comment 19

[Ian Neal]

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

Comment 20

[Ian Neal]

This also seems to be affecting Phoenix (bug 179954) and Chimera (bug 180054)
would this UNICODE patch caused the problem in those browsers too?

Comment 21

[Matthias Versen [:Matti]]

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

Comment 22

[Roy Yokoyama]

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

Comment 23

[Roy Yokoyama]

No. Unicode patch is only for Windows platforms.
It doesn't cause any problems for other platform.s

Comment 24

[Roy Yokoyama]

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.

Comment 25

[Ian Neal]

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.

Comment 26

[rubydoo123]

Ian: can you try upgrading to Flash 6 and see if that does indeed prevent the
crashes on the other builds?

Comment 27

[Ian Neal]

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.

Comment 28

[Ian Neal]

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

Comment 29

[Matthias Versen [:Matti]]

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

Comment 30

[Ian Neal]

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

Comment 31

[Olivier Cahagne]

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

Comment 32

[Roy Yokoyama]

It looks like the change 
from SetWindowLong() to SetWindowLongW() causes to crash.

Comment 33

[Peter Lubczynski]

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?

Comment 34

[av (gone)]

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

Comment 35

[Roy Yokoyama]

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.  
 

Comment 36

[av (gone)]

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.

Comment 37

[Roy Yokoyama]

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.

Comment 38

[Matthias Versen [:Matti]]

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

Comment 39

[av (gone)]

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?

Comment 40

[Markus Gerstel]

This bug should either be fixed for 1.2 or mentioned in the release notes

Comment 41

[Roy Yokoyama]

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

Comment 42

[Roy Yokoyama]

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

Comment 43

[Markus Gerstel]

Just tested - 1.2 is not affected. Thx.

Comment 44

[Roy Yokoyama]

peter:  do we have document for the flash 6?
        I'd like to see what has changed from 4/5 to 6.

Comment 45

[av (gone)]

Roy, should not we also define MSVC var for unicode on Windows projects?
Something like:

DEFINES += -DMOZ_UNICODE -DUNICODE

Comment 46

[Roy Yokoyama]

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

Comment 47

[av (gone)]

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?

Comment 48

[Roy Yokoyama]

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

Comment 49

[Blake Ross]

This is blocking a Phoenix release -- any eta?

Comment 50

[Roy Yokoyama]

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?

Comment 51

[rubydoo123]

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.

Comment 52

[Peter Lubczynski]

I think unicode support was added in Flash 6. I imagine the 'fix' is to upgrade. 
cc:ing emillard for insight.

Comment 53

[Roy Yokoyama]

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.

Comment 54

[Matthias Versen [:Matti]]

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

Comment 55

[Ed Millard]

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. 

Comment 56

[Roy Yokoyama]

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

Comment 57

[Max Kanat-Alexander]

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

Comment 58

[Michael Lefevre]

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

Comment 59

[R.K.Aa.]

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

Comment 60

[Matthias Versen [:Matti]]

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

Comment 61

[av (gone)]

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.

Comment 62

[Peter Lubczynski]

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

Comment 63

[Roy Yokoyama]

beppe: Can I get some help from Macromedia team?  I am also out of
       ideas.  

Comment 64

[Ed Millard]

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.  

Comment 65

[Peter Lubczynski]

roy: re: comment #53, how can we dynamically switch off unicode for [some] plugins?

Comment 66

[Pratik]

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

Comment 67

[Roy Yokoyama]

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.

Comment 68

[Roy Yokoyama]

Attachment: nsWindow::WindowClass() needs to return the classname instead of null

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

Comment 69

[Roy Yokoyama]

Never mind, peter.  It still crashes..... :(

Comment 70

[Roy Yokoyama]

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.

Comment 71

[Roy Yokoyama]

Attachment: Give an unicode option when SubClassing the widget window

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?

Comment 72

[Peter Lubczynski]

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.

Comment 73

[av (gone)]

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.

Comment 74

[Roy Yokoyama]

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

Comment 75

[Phillip Rhodes]

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

Comment 76

[Roy Yokoyama]

Attachment: define new mUnicode in nsWidgetInitData

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?

Comment 77

[Roy Yokoyama]

Silly me, I forgot to check for nsnull for nsWidgetInitData *aInitData.
I'll post a new patch in a minute.

Comment 78

[Roy Yokoyama]

Attachment: revised

Peter: can you review?

Comment 79

[av (gone)]

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?

Comment 80

[Roy Yokoyama]

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

Comment 81

[av (gone)]

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.

Comment 82

[Roy Yokoyama]

I think we need input from Peter about this. Peter?

Comment 83

[Ashley Bischoff (blog at handcoding.com)]

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

Comment 84

[Matthias Versen [:Matti]]

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

Comment 85

[Asa Dotzler [:asa]]

Adding the blocking1.3a+ flag as this is wanted by [e-mail] for
Mozilla 1.3a scheduled to freeze on Dec 4. 

Comment 86

[Chris Lyon]

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

Comment 87

[Peter Lubczynski]

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.

Comment 88

[Roy Yokoyama]

Attachment: add mListenForResizes = PR_TRUE;

peter: can you verify the case where we use full-page plugins? 
       and review? I want to checkin this patch today.	Thanks

Comment 89

[av (gone)]

This version looks fine to me.

Comment 90

[rubydoo123]

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

Comment 91

[Patrick]

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 92

[Peter Lubczynski]

Comment on attachment 107929 [details] [diff] [review]
add mListenForResizes = PR_TRUE;

r=peterl

Comment 93

[Roy Yokoyama]

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

Comment 94

[kinmoz]

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?

Comment 95

[av (gone)]

Would it make sense to mimic the logic in |nsView::CreateWidget| trying to get
the parent and then do the same thing?

Comment 96

[kinmoz]

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.

Comment 97

[Peter Lubczynski]

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.

Comment 98

[Roy Yokoyama]

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 99

[kinmoz]

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.

Comment 100

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 101

[Roy Yokoyama]

Attachment: removing mListenForResizes

thanks, peter, av and kin.

Comment 102

[Roy Yokoyama]

Comment on attachment 108107 [details] [diff] [review]
removing mListenForResizes

carry over reviews with 
permission

/r=peterl
/sr=kin

Comment 103

[Roy Yokoyama]

checked into the trunk.

Comment 104

[shrirang khanzode]

looks good to me. I did not crash on XP with today's trunk 1204. Reporter, pls
reverify that it fixes your crash. thx!!

Comment 105

[Peter Lubczynski]

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