149336 - Linux Flash crashes and causes stack curruption in NPP_New when swLiveconnect=TRUE [Crash loading www.joins.com] [@ 0x00000000 - nsTableCellFrame::Reflow]

Status: VERIFIED FIXED

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

Description

[Tracy Walker [:tracy]]

seen on linux commercial branch build 2002-06-05-07-1.0.0

-load www.joins.com
the page loads veryu slowly. it takes a few minutes, but the app eventually crashes.

unfortunately, the talkback incidents I submitted aren't showing up in the database.

Comment 1

[Tracy Walker [:tracy]]

this is an international topsite.

Comment 2

[Roy Yokoyama]

any other platforms?
cc i18n folks.

Comment 3

[Tracy Walker [:tracy]]

mac osx and windows builds from this morning do not crash.

Comment 4

[Roy Yokoyama]

this site again, shhh.  All crashing bugs (83670, 105127, 126229, 109356)
resulted to WFM. 
Can anyone else reproduce this?

Comment 5

[Yuying Long]

I can reproduce it on 06-05 branch build, I saw the crash after finished load
the page.  Somehow I can not find the talkback ID right now.

However, with 06-04 branch build I didn't get this crash, a regression from
yesterday?

Comment 6

[Roy Yokoyama]

Any stack trace/talkback help here since I have no linux.

shanjian/ftang/smontagu: I need help on Linux platform.

Comment 7

[Roy Yokoyama]

WFM with smontagu's debug build in Linux. 
His machine didn't have the plugin. 
tracy/ylong: do you have the plugin installed?

Comment 8

[Tracy Walker [:tracy]]

this all the talkback incident has:

Incident ID 7032910    Stack Signature     Email Address  [e-mail] 
  Product ID  Gecko1.0    Build ID  2002060507    Trigger Time  2002-06-05
10:27:14    Platform  LinuxIntel    Operating System  Linux 2.2.12-20smp   
Module     URL visited  www.joins.com    User Comments   crashed loading
www.joins.com   Trigger Reason  SIGSEGV: Segmentation Fault: (signal 11)   
Source File Name     Trigger Line No.     Stack Trace  

no stack trace present. :-/

flash plugin is installed and works on other sites

Comment 9

[Roy Yokoyama]

>WFM with smontagu's debug build in Linux. 
My bad.  We were looking at the today's trunk build.
After downloading the branch, we were able to reproduce the crash.
Simon is pulling the branch source now.

Comment 10

[Roy Yokoyama]

nsDocumentViewer.cpp change is the only checkin
I can think of may cause this problem. Bug 132672
Other checkins are mail/news specific.

cc [e-mail]

Comment 11

[Roy Yokoyama]

smontagu/shanjian: can I ask you to rollback rod's change (Bug 132672) on
                   your linux branch build?

Comment 12

[Simon Montagu :smontagu]

I can't reproduce this in a mozilla branch build either with or without the
plugin. I should soon have a commercial branch debug build for testing.

Comment 13

[Simon Montagu :smontagu]

... but I can reproduce the crash with a downloaded mozilla branch build,
BuildID 2002060507

Comment 14

[Peter Lubczynski]

If anyone has an updated branch build, could we try backing out Rod and see if
that makes the crash go away?

Comment 15

[serge (gone)]

I doubt rods chechin causes this crash,
here is the simple test I've tried:
the patch http://bugzilla.mozilla.org/attachment.cgi?id=84435&action=view
changed only libgkcontent.so, and there is no more changes in libgkcontent.so,
I swapped that lib between 2002-06-04-11-1.0.0 & 2002-06-05-07-1.0.0 builds,
the first one is still stable, when last one is still crashing:(

Comment 16

[Peter Lubczynski]

Is this an old bug? Did this ever work correctly?

Comment 17

[rods (gone)]

You can easily "back out" my checkin by commenting out lines 1372 & 1373 in the
Documentviewer.cpp
http://lxr.mozilla.org/mozilla1.0/source/content/base/src/nsDocumentViewer.cpp#1372

But I don't think my check in has anything to do with the crash, I can load the
URL with the trunk and without the flash plugin, but that probably isn't a very
meaningful test.

Comment 18

[rods (gone)]

Can you attach a stack trace now that you can get it to crash?

Comment 19

[Yuying Long]

The crash in this site is an old problem.  We never had a solution to fix it. 
And because of this web page contains changes everyday, so once a while, we have
this crash and then after a few days, then the crash gone away. 

Comment 20

[Simon Montagu :smontagu]

If the problem was with the site content, we would be seeing the crash when
testing today with both yesterday's build and today's build. Is that the case?

Comment 21

[Yuying Long]

With same web page content(today), it crashes on today's build but not
yesterday's build for me and Tracy.

Comment 22

[serge (gone)]

it could be plugins problem bug 143178, I'm bulding debug build now

Comment 23

[Rui Xu]

Reassign to the correct IQA owner: [e-mail]
and feel free to ask us for any help.

Comment 24

[Shanjian Li]

I could not reproduce the problem with my branch debug build (today's pull).
Base one ylong's information, we might want to later this one and open the tree.  

Comment 25

[Simon Montagu :smontagu]

I still haven't succeeded in getting a stack trace, because I can't reproduce
consistently in a debug build, but I got some information from running a
downloaded build in debugger:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1024 (LWP 10910)]
0x4153f176 in NSGetModule () from
/u/smontagu/moz_0605/mozilla/components/libgklayout.so
(gdb) 

Comment 26

[Simon Montagu :smontagu]

Attachment: Finally a stack trace

Comment 27

[Peter Lubczynski]

hm...stack points to Reflow....but I think there are some other bugs just like
this one. I'm not convinced this is a new problem but it's strange yesterday's
builds work. Maybe some curruption of sorts?

Anyone who can reproduce this in a debug branch build can you try backing out Rod?

Comment 28

[Simon Montagu :smontagu]

I backed out Rod and haven't crashed since, but this may be a false lead because
I didn't crash every time before.

Comment 29

[serge (gone)]

I cannot crash branch debug build with plugins or w/o plugins all just WFM,
I did surf joins.com for about 15+ min:(

Comment 30

[Roy Yokoyama]

serge: Is your build with or without rod's patch?

Comment 31

[Adam Hauner]

Bugs with nsBlockFrame::ReflowDirtyLines - bug 117088, bug 120165, bug 134917 -
all marked as WFM.

Comment 32

[rods (gone)]

I just pulled the branch on Linux w/o the plugin is loads fine, no crash. I
haven't installed the plugin yet.

Comment 33

[Kai Engert [:KaiE:]]

If I try really hard, click a lot in that page, resize a lot, move the popup
window around while the page below it is loading, reload the page, have two
windows open and close, and stuff, I can make it crash after about 5 minutes
heavy usage.

While during all my tests I was running it in the debugger, I have problems
reporting a crash location.

Once the debugger was unable to give out a crash location, it just displayed
garbage.

Next, I was shown a crash somewhere in the reflow code. It crashed in a
variable, where the first parameter was "aState", given as a reference (not as a
pointer). It crashed when trying to assign to aState.X, IIRC. I can't tell
exactly, because the debugger crashed while I was looking around. However, it
seemed to me, that it might have been wrong information, because it seemed that
reference was passed on from a caller, and in that caller the variable did not
point to zero.

Next, I'm not in the debugger again, and it crashed at a completely different
location. Argh! Sorry, I was about to copy the stack from the window, but now
the debugger is again in an endless loop, no longer allowing me to control it.

But I can say that it was unable to lock a mutex, return code 22.
nsWebShellWindow::FirePersistenceTimer.

Does all of that indicate memory corruption? Normally I'm able to debug without
problems.

Linux branch build from today, flash plugin installed.

Comment 34

[Kai Engert [:KaiE:]]

I forgot to say, when I crashed in the reflow code, that aState reference
variable did point to zero.

Comment 35

[hugo van woerkom]

For what it's worth: this site crashes my CVS MOZILLA_1_0_RELEASE build, but
*not* the CVS trunk build of 5/27 (ID 2002052722) that I have.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

ReflowDirtyLines - waterson's been in there a bit recently.  Adding myself and a
few reflow people.

Comment 37

[Roy Yokoyama]

I don't think I should be the owner of this bug. I dont' even
have linux mahcine.  Assign to smontagu.

Comment 38

[Darin Fisher]

so i just pulled and built the latest branch bits under redhat 7.2.  i installed
the latest flash/shockwave plugin and could not reproduce this crash.

why is this a smoketest blocker?

Comment 39

[Darin Fisher]

after loading the site a couple more times i observed both a hang and a crash. 
the stack trace after the crash seems odd...

#0  __pthread_mutex_lock (mutex=0x410) at mutex.c:99
#1  0x40659be8 in __libc_free (mem=0x854e398) at malloc.c:3152
#2  0x43137215 in morkUsage_g_static_init_done () from
/export/darin/mozilla/dist/bin/plugins/libflashplayer.so
#3  0x41f14500 in compositeClassRec () from /usr/X11R6/lib/libXt.so.6

this particular stack trace seems to implicate the flash plugin... but it might
just be the victum instead of the culprit.

Comment 40

[Darin Fisher]

verified the crash is not present in the 2002-06-04-14 linux build but is
present in the 2002-06-05-07 linux build.  looking now to see what changed...

Comment 41

[Darin Fisher]

i tried backing out changes by av, rods, and nhotta and that didn't eliminate
the crash.  80% of the time i crash in nsObjectFrame::Destroy calling
nsIPluginInstance::Stop... there appears to be heap corruption because the libc
free call typically isued by libflashplayer results in either a crash or an
infinite loop.

out of the list of checkins between 6/4/14 and 6/5/7, there doesn't appear to be
anything obvious that might have caused this.

at this point i'm somewhat stuck... the only thing i can think to do is to
simply backout everything one-by-one.  what a pain!!

Comment 42

[Frank Tang]

darin- I appreciate if you can take this bug.  simon's experties is on Window
and he is just start to learn Linux debugging right now. He is the wrong person
to own this issue. Reassign to me if you do not agree to take it. Thanks

Comment 43

[Frank Tang]

cc av since darin mention about object frame. 

also, make sure you create a new profile when you try to reproduce it. with
smontague's machine, we cannot reproduce it once the 6/4 load the page in a same
profile. After that, we cannot reproduce it on the 6/5 build with the same
profile. (could be because some content are cached). darin, do you have any
trick to reproduce it? which versoin of linux are you on? redhat 7.2 7.3 ? 

Comment 44

[Darin Fisher]

i was testing this on a redhat 7.2 system.  i was able to reproduce the problem
almost always when reloading www.joins.com... only sometimes did it happen on
initial load.

i know nothing about the related code... i was only looking at this bug because
i happened to have a linux branch build and because this bug was preventing me
from checking in changes to the branch.  unfortunately, i don't have much time
to look at this today... i will be able to spend some time on this come monday.

Comment 45

[av (gone)]

My check in on 06.04.2002 causes doing some new things in cases when a plugin is
not installed. When plugin is present it does not change anything.

Windows build gives the followin warning of every plugin instantiation, if this
can be of any help:

WARNING: empty damage rect: update caller to avoid fcn call overhead, file
y:\branch\mozilla\layout\html\base\src\nsFrame.cpp, line 2287

Comment 46

[Samir Gehani]

Serge will be looking into this.  Thanks Serge!

Comment 47

[Chris McAfee]

Two things, 
 1) can we save off a static version of this site so it doesn't change on us?
 2) maybe it's time to pull-by date search through the checkins to figure
    this one out.  It's painful, but we've done this many times before to
    find hard-to-find bugs like this.

Comment 48

[serge (gone)]

finally I got the stack trace, here is top part of it:
(gdb) bt
#0  0x4240ef8e in nsTableFrame::ReflowChildren (this=0x8a39c08,
aPresContext=0x96761e8, aReflowState=@0xbfffb720, aDoColGroups=1, aDirtyOnly=0,
aStatus=@0x0, aLastChildReflowed=@0xbfffb7b8, aReflowedAtLeastOne=0x0) at
nsTableFrame.cpp:3372
#1  0x4240c5bf in nsTableFrame::ReflowTable (this=0x8a39c08,
aPresContext=0x96761e8, aDesiredSize=@0xbfffbae0, aReflowState=@0xbfffb8b0,
aAvailHeight=1073741824, aReason=eReflowReason_Resize,
aLastChildReflowed=@0xbfffb7b8, aDoCollapse=@0xbfffb7f0,
aDidBalance=@0xbfffb7e8, aStatus=@0x0) at nsTableFrame.cpp:2301
#2  0x4240c00c in nsTableFrame::Reflow (this=0x8a39c08, aPresContext=0x96761e8,
aDesiredSize=@0xbfffbae0, aReflowState=@0xbfffb8b0, aStatus=@0x0) at
nsTableFrame.cpp:2160
#3  0x4230802f in nsContainerFrame::ReflowChild (this=0x8a399e8,
aKidFrame=0x8a39c08, aPresContext=0x96761e8, aDesiredSize=@0xbfffbae0,
aReflowState=@0xbfffb8b0, aX=0, aY=0, aFlags=3, aStatus=@0xbfffc084) at
nsContainerFrame.cpp:784
#4  0x4241e9b7 in nsTableOuterFrame::OuterReflowChild (this=0x8a399e8,
aPresContext=0x96761e8, aChildFrame=0x8a39c08, aOuterRS=@0xbfffbf10,
aMetrics=@0xbfffbae0, aAvailWidth=0x0, aDesiredSize=@0xbfffbb60,
aMargin=@0xbfffbb50, aMarginNoAuto=@0xbfffbb40, aPadding=@0xbfffbb30,
aReflowReason=eReflowReason_Incremental, aStatus=@0xbfffc084) at
nsTableOuterFrame.cpp:1026
#5  0x4241f836 in nsTableOuterFrame::IR_InnerTableReflow (this=0x8a399e8,
aPresContext=0x96761e8, aOuterMet=@0xbfffc1c8, aOuterRS=@0xbfffbf10,
aStatus=@0xbfffc084) at nsTableOuterFrame.cpp:1342
#6  0x4241ed69 in nsTableOuterFrame::IR_TargetIsInnerTableFrame (this=0x8a399e8,
aPresContext=0x96761e8, aDesiredSize=@0xbfffc1c8, aReflowState=@0xbfffbf10,
aStatus=@0xbfffc084) at nsTableOuterFrame.cpp:1132
#7  0x4241ec63 in nsTableOuterFrame::IR_TargetIsChild (this=0x8a399e8,
aPresContext=0x96761e8, aDesiredSize=@0xbfffc1c8, aReflowState=@0xbfffbf10,
aStatus=@0xbfffc084, aNextFrame=0x8a39c08) at nsTableOuterFrame.cpp:1104
---
notice:  aStatus=@0x0 in frame 2 but aStatus=@0xbfffc084 in all frames below
frame 2, some how it's getting corrupted in nsContainerFrame::ReflowChild()
and I crashed on assignment to aStatus.

ccing karnaze

Comment 49

[serge (gone)]

I've played for a while (heavily resizing & reloading joins.com) with branch
build 2002060411, which is a more stable compare the latest ones, I've submitted
3 TB reports, all have different stack traces, only one is plugin related:
http://climate.netscape.com/reports/incidenttemplate.cfm?setvar=DeveloperDeveloperTabSet:Overview+%28Developer%29&bbid=7125026#DeveloperMachineState
http://climate.netscape.com/reports/incidenttemplate.cfm?setvar=DeveloperDeveloperTabSet:Overview+%28Developer%29&bbid=7124802#DeveloperMachineState
http://climate.netscape.com/reports/incidenttemplate.cfm?setvar=DeveloperDeveloperTabSet:Overview+%28Developer%29&bbid=7124620#DeveloperMachineState
and here is TB report from 2002060707 which has corrupted stack trace,
http://climate.netscape.com/reports/incidenttemplate.cfm?setvar=DeveloperDeveloperTabSet:Code+Around+the+PC&bbid=7124007#DeveloperMachineState
but EIP points to
/builds/release/branch/2002-06-07-07-1.0.0/components/libgklayout.so  0x406d2000
 0x408b5333

I'm lowering severity for this bug to critical, and continue to investigate.

Comment 50

[Darin Fisher]

i think the various stack traces result from heap corruption.  so, the stack
traces unfortunately don't really point us at the real problem... we need to
figure out where the heap corruption first occurs.  so far, we're only seeing
victimized bits of code dying as a result of someone else's badness (i think).

Comment 51

[Roland Mainz]

Can anyone run a session with Rational Purify and look for FMW (Free Memory
Writes) etc., please ?

Comment 52

[Darin Fisher]

Roland: this is linux only... isn't Rational Purify only available under Windows
and Solaris?  would love to have a tool like this available under linux :)

Comment 53

[Roland Mainz]

Darin Fisher wrote:
> Roland: this is linux only... isn't Rational Purify only available under 
> Windows and Solaris?

I would be surprised if this crash does not occur under Solaris, too.
Note that pavlov has a couple of nice Solaris machines... =:-)

> would love to have a tool like this available under linux :)

Does Linux come with "watchmalloc" ?

Another possible solution under Linux may be "Valgrind"
(http://developer.kde.org/~sewardj/) but I never had the time to test it myself
in detail...

Comment 54

[Peter Lubczynski]

We've got quite a few bugs marked WFM that crash on joins.com. It doesn't look
like any of them have anything helpful though :(

Comment 55

[Roland Mainz]

Peter Lubczynski wrote:
> We've got quite a few bugs marked WFM that crash on joins.com. It doesn't look
> like any of them have anything helpful though :(

I guess the content from joins.com is dynamically generated and (maybe) differs
based on the type of client, locale and used OS...

Maybe we can ask the staff which does the "smoketest" testing to monitor this
issue and do debugging on the same machine where it occurs...

Comment 56

[av (gone)]

Even if Purify is not available on a platform of interest, it still could be
helpful to run it on Windows. The thing can be that corruption occurs there too
but Windows just survives.

Comment 57

[Roland Mainz]

Just did some testing with my Solaris builds, joins.com does not crash it ad-hoc
but sooner or later it crashes while loading other pages like
http://www.mozilla.org/ or http://www.spiegel.de/

Running Purify on Solaris should give some hints what's going wrong...

Comment 58

[Tracy Walker [:tracy]]

I was able to reproduce this with ease until today.  The only thing different
about the test machine is that it has been rebooted since the series of crashes
seen at this sight during last weeks smoketesting.  I shut the machine off for
the weekend. 
Also of note: a slow mail issue with this same machine is also gone today.

Comment 59

[Roland Mainz]

BTW: Solaris has another usefull tool to track FMRs:
Try this:
% export LD_PRELOAD=libmapmalloc.so
% ./mozilla -g

The "mapmalloc" shared library then replaces the normal |malloc()| allocator
with a function which uses mmap() for obtaining memory.
A side-effect is that we get more or less unique memory addresses which are
surrounded by areas of non-mapped memory, e.g. writing beyond the area of the
allocated space will result in a SEGV. |free()|'ing the memory will unmap it,
any further read/write access to that memory will result in a SEGV.

Comment 60

[serge (gone)]

well, the things were changed (mozilla code + joins.com content),
I cannot reproduce the crash with 20020610 branch builds, neither release one
nor debug:(

Comment 61

[Darin Fisher]

i can still crash easily with the 1.0-20020605-07 build under linux.

Comment 62

[Darin Fisher]

i'm able to reproduce the crash using just TestGtkEmbed which nicely eliminates
a number of checkins.

Comment 63

[Darin Fisher]

i was also able to repro the crash after removing all of the security libraries
from my build.

Comment 64

[Peter Lubczynski]

Ran joins.com through Purify on Windows and nothing interesting showed up. :(

Does an older version of the Flash plugin work better?

Comment 65

[Darin Fisher]

i'm doing an electric fence build now FWIW.

Comment 66

[serge (gone)]

hmm, it appears my debug build of  TestGtkEmbed is linked against
old mozilla libs, which came with red hat distribution, and reside in /usr/lib
/usr/lib/mozilla dirs.
Darin, if you have /usr/lib/libgtkembedmoz.so[libgtksuperwin.so] installed, you
are probably linking against those libs too:(

Comment 67

[Darin Fisher]

nope... ldd tells me that i'm picking up the correct version of libmozembedgtk.

Comment 68

[Peter Lubczynski]

Okay, I'm seeing something really weird on Windows which could be causing this
problem:

Setting a breakpoint in ns4xPluginInstance before we call NPP_NewProc, I
sometimes see us trying to use the default plugin on a mime type of
"application/x-ableclick-dottag". This only happens sometimes, on random
reloads. This could mean Andrei's patch is being touched sometimes.

Also setting a breakpoint before calling NPP_SetWindow, I sometimes see us
passing in width == height == 0 after a previous call with values for
width/height. It might be a long shot, but I wonder if Flash is getting confused
because of the multiple calls with differenet values?

Comment 69

[Darin Fisher]

peterl: i tried backing out av's change and it still crashed.

Comment 70

[Peter Lubczynski]

Setting env var NSPR_LOG_MODULES=Plugin:8,PluginNPN:8,PluginNPP:8 will output
some plugin debug info.

Comment 71

[Darin Fisher]

unfortunately, my ElectricFence build doesn't run.  during startup it complains
about insufficient memory.  even with 1GB of RAM and what i would consider
plenty of swapspace it continues to fail.  oh well :(

Comment 72

[rubydoo123]

Serge and AV has checked that this is not the cause of Andrei's checkins,
assigning back to darin

Comment 73

[Darin Fisher]

targeting 1.0.1 since this is a branch bug not a trunk bug.

Comment 74

[ly nguyen]

i found a stack trace that matched simon's stack trace (incident #7091063).
examining a talkback list with the same stack sig, looks like there are 3
incidents for moz1.0 where win machines are crashing, 9 incidents for rc3 where
linux and win machines were crashing (mostly linux though), and 1 incident under
pr1 for a win machine. this stack sig dates all the way back to build
2000110801. take note that the list only conveys stack sigs as a common thread
not the entire stack trace. will take a closer look...

Comment 75

[ly nguyen]

Attachment: values of locals and params

from talkback info and the stack trace that matched simon's, here's the values
of locals and params.

Comment 76

[Darin Fisher]

so, i switched over to the latest branch build and a single processor machine.
for some reason, i'm now able to repro the crash nearly 100% of the time... only
now the crash happens in nsBlockFrame::DoReflowInlineFrames.  it crashes because
|this| becomes null.  i inspected the code, added some ASSERTIONS checking for a
null this pointer, and it looks like the culprit is stack corruption.

so, with dbaron's help i discovered that the source of the problem is actually
stack corruption resulting from the invocation of CallNPP_NewProc from
ns4xPluginInstance::InitializePlugin.  we pushed a large buffer onto the stack
and populated it with known values.  then we checked the contents of that buffer
before and after the call to CallNPP_NewProc, and discovered corruption upon
return from CallNPP_NewProc.

i'll attach a patch that demonstrates the problem.

Comment 77

[Darin Fisher]

Attachment: patch to demonstrate stack corruption

this patch allocates a 256 byte buffer on the stack inside
nsBlockFrame::ReflowInlineFrames and populates it with uniformly with the ASCII
'a' character.

it then verifies the values in this buffer before and after invoking
CallNPP_NewProc inside ns4xPluginInstance::InitializePlugin.  after the call to
CallNPP_NewProc, the stack buffer is corrupt.

without this stack buffer, the |this| pointer of nsBlockFrame get's nulled out.


chances are you might not be able to reproduce this bug using this patch, but
hopefully this information will mean something to the plugin engineers.

Comment 78

[Darin Fisher]

-> plugins

Comment 79

[serge (gone)]

confirm linux flash 5.0 r48 & 4.0 r12 plugins corrupt the stack if
swliveconnect="TRUE" argument is present:(

Comment 80

[rubydoo123]

handing this back to Serge for debug

Comment 81

[Frank Tang]

My question is what changes we made in the tree in 06-05 to trigger this problem
by flash ?
or the problem is always but corrupt the stack differently which does not
introduce crash?

Comment 82

[serge (gone)]

the problem is always, I've seen it with flash 4.0, but some how it's getting
exposed randomly, and probably depends on stack layout, in my observation it
always zeroes something like couple of 4 bytes addresses, in some cases it could
harmless.

Comment 83

[Darin Fisher]

exactly!  i also saw it nulling out one DWORD.  on my machine it just happens to
null out the |this| pointer for a nsBlockFrame.  pure coincidence.

Comment 84

[serge (gone)]

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

Comment 85

[serge (gone)]

Attachment: patch v1.

this is the hack, but what else I can do:(

Comment 86

[Roland Mainz]

Comment on attachment 88356 [details] [diff] [review]
patch v1.

Can you add an env var to turn this hack _off_, please ?

Maybe the plugin vendor fixes the issue some day and this hack may then be
incompatible with the fixed plugin...

Comment 87

[serge (gone)]

I doubt, it'll be ever fixed, they probably hacked LiveConnect support in ns4.x,
but surely I can add hidden pref to disable this hack, or env var is preferable?

Comment 88

[serge (gone)]

Attachment: patch v2 w/ env var to disable this hack

added check for env var MOZ_DISABLE_FLASH_SWLIVECONNECT_HACK

Comment 89

[Roland Mainz]

serge wrote:
> I doubt, it'll be ever fixed, they probably hacked LiveConnect support in 
> ns4.x, but surely I can add hidden pref to disable this hack,
> or env var is preferable?

Mhhh, I am not sure... when I hit such problems in the past I always used env
vars since they are less AFAIK expensive than the prefs service.
Choose something you like - but do not forget to add a comment in the release
notes! :)

Comment 90

[serge (gone)]

Attachment: patch v3

call getenv() only for "application/x-shockwave-flash" mimetype

Comment 91

[Darin Fisher]

Comment on attachment 88360 [details] [diff] [review]
patch v3

>Index: ns4xPluginInstance.cpp

>+  if (count && !PL_strncmp(mimetype, flashMimeType, sizeof(flashMimeType)-1)) {

beware this usage of PL_strncmp... it will also succeed if mimetype equals...

  "application/x-shockwave-flash-just-kidding"

strcmp is probably more appropriate.

Comment 92

[serge (gone)]

Attachment: patch v4

agreed, the same is true for 
+	 if (!PL_strncasecmp(names[i], blockedParam, sizeof(blockedParam)-1)) {


all PL_srtn changed to PL_strcasecmp()

Comment 93

[av (gone)]

Comment on attachment 88360 [details] [diff] [review]
patch v3

r=av

Comment 94

[av (gone)]

Comment on attachment 88369 [details] [diff] [review]
patch v4

commented in the wrong attachment, sorry. r=av should be here.

Comment 95

[Roland Mainz]

Three minor, non-mandatory nits:
- env vars should be build like this MOZILLA_%name_of_module%_%description%
(most of the code does that like this), e.g.
s/MOZ_DISABLE_FLASH_SWLIVECONNECT_HACK/MOZILLA_PLUGIN_DISABLE_FLASH_SWLIVECONNECT_HACK/
- Please use |PR_GetEnv()| instead of |getenv()|
- What about caching the result of |PR_GetEnv()|/|getenv()| ? Or is that place
not performance-critical ?

Comment 96

[serge (gone)]

Attachment: patch v5

With Roland's concerns addressed. Thanks.

Comment 97

[Roland Mainz]

Comment on attachment 88382 [details] [diff] [review]
patch v5

r=[e-mail]

Comment 98

[serge (gone)]

Darin, could you sr= ,please?

Comment 99

[rubydoo123]

this should have been marked as nsbeta1+ already, adding ADT1 - this is a topcrash

Comment 100

[Darin Fisher]

Comment on attachment 88382 [details] [diff] [review]
patch v5

>Index: ns4xPluginInstance.cpp

>+#ifdef XP_UNIX

>+      int cnt = (int) count;
>+      for (int i=0; i<cnt; i++) {

why do you need |cnt|?	how about this instead:

	for (PRUint16 i=0; i<count; i++) {


>+        if (!PL_strcasecmp(names[i], blockedParam)) {
>+          char *val = (char*) values[i];
>+          if (val && *val) {
>+            // we cannot just *val=0, it wont be free properly in such case
>+            val[0] = '0';
>+            val[1] = 0;
>+          }

|values| is declared |const char* const*| meaning that you shouldn't be
allowed to alter it, so is this code really valid?  what if |values|
contains pointers to string literals?  wouldn't this cause a crash?

Comment 101

[serge (gone)]

1. (PRUint16 i=0; makes sense 
2. |const char* const* values| is alloced at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/base/src/nsObjec
tFrame.cpp&rev=1.349&root=/cvsroot#3020
and freed at line #2096
so, it couldn't be a const ptr to string literal,
of course, I can dup all of name/value strings modify and free it, but I think 
it would be unnecessary performance hit, correct me if I'm wrong, please.

Comment 102

[Darin Fisher]

serge: it just looks like a bug waiting to happen.  why prototype the variable
as |const char * const| if you really intend to modify it?  it just seems scary
to do so without at least adding a big fat warning and comments to the effect
that you know where this data is coming from.  but even with that, i'd worry.

Comment 103

[Andrew Schultz]

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

Comment 104

[Jaime Rodriguez, Jr.]

lowering impact to adt2 RTM.

Comment 105

[serge (gone)]

Attachment: patch v6

All right, I've moved this hack into
nsObjectFrame.cpp:nsPluginInstanceOwner::EnsureCachedAttrParamArrays()
"swliveconnect" is getting removed from attributes list completely.
please review.

Comment 106

[av (gone)]

I don't think we need checking prefs or env to disable the hack. If Flash is
going to be scriptable with Mozilla this parameter will no longer be relevant
anyway and they will likely just ignore it in their code. All this is because
scriptability can be achieved a different way, and LiveConnect is deprecated for
this purpose. 

I vote for just ignoring it if it causes problems, this will make the code much
cleaner.

Comment 107

[serge (gone)]

Attachment: patch v7

I agreed with av, here is the patch

Comment 108

[Peter Lubczynski]

Comment on attachment 88980 [details] [diff] [review]
patch v7

This patch won't catch cases where the TYPE attribute is missing and we guess
via extension or HTTP header. eg:

<EMBED SRC="myflash.swf" swLiveconnect=true>

I like patch v.5 better. It's only because of the deprecated XPCOM API that we
use const pointers.

Comment 109

[serge (gone)]

Attachment: patch v8

Peter is right, w/o type= we'll missed swliveconnect= :(
This patch is the same a patch v5, with BIG FAT WORNING added
Darin, would you agree on this worning?

Comment 110

[Darin Fisher]

Comment on attachment 89001 [details] [diff] [review]
patch v8

sr=darin

Comment 111

[serge (gone)]

on the trunk
mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp,v  <-- 
ns4xPluginInstance.cpp
new revision: 1.95; previous revision: 1.94
Thanks all.

Comment 112

[shrirang khanzode]

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

Comment 113

[shrirang khanzode]

verified on 0625 trunk on linux, this no longer crashes, works great.

Comment 114

[serge (gone)]

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

Comment 115

[scottputterman]

adding adt1.0.1+.  Please get drivers approval before checking in.

Comment 116

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 117

[serge (gone)]

on the branch
mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp,v  <-- 
ns4xPluginInstance.cpp
new revision: 1.90.2.4; previous revision: 1.90.2.3

Comment 118

[serge (gone)]

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

Comment 119

[rubydoo123]

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

Comment 120

[shrirang khanzode]

verfd fixd, no crash.

Comment 121

[Jay Yan]

.