[Webshell] test for webshell leaking?

VERIFIED FIXED in M15

Status

SeaMonkey
Build Config
P1
critical
VERIFIED FIXED
19 years ago
a year ago

People

(Reporter: Bruce Mitchener, Assigned: Nisheeth Ranjan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT-] need help to verify plus for beta2)

Attachments

(1 attachment)

(Reporter)

Description

19 years ago
It was discussed during one of the previous rounds of beeting on nsWebShell
leaks that there should be a way to track the number of allocations and
deallocations of nsWebShells and then at exit time, print an error notice if
some were leaked.  This could be tied into the smoke tests as well so that this
type of thing became very noticable very quickly.

Thoughts?
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
Summary: test for webshell leaking? → [Webshell] test for webshell leaking?
Target Milestone: M10
(Assignee)

Comment 1

19 years ago
We probably need to implement an observer that gets notified on webshell
allocations and deallocations and prints out its tally at exit time.  This
should be relative simple to do.  Setting milestone to M10.  I'm away on a
seminar for most of M9.
(Assignee)

Comment 2

19 years ago
Bul re-assigning webshell bugs to the new webshell owner, Scott Collins.  Scott,
please adjust the target milestone assignments as you see fit.  Thanks.
(Reporter)

Updated

19 years ago
Assignee: scc → chofmann
Severity: normal → critical
Status: ASSIGNED → NEW
(Reporter)

Comment 3

19 years ago
if scc no longer owns webshell, then this needs a new owner.  this is critical
for us to not start leaking the webshell again once it gets fixed.  I understand
that there is now code in the tree to do this, but it isn't turned on, and it
isn't tied into the tinderbox smoke tests (or daily verifications).

Updated

19 years ago
Target Milestone: M10 → M12

Comment 4

19 years ago
this is a small amount of work to help avert huge problems.  Should be done
ASAP.  Setting to M12.  If the code already exists, it should be turned on and
the bug should be assigned to somebody on the build team to get it hooked up to
tinderbox, or at least QA should verify the output during daily tests.

Updated

19 years ago
Summary: [Webshell] test for webshell leaking? → [BETA] [Webshell] test for webshell leaking?

Comment 5

19 years ago
nisheeth, do you have the code for this or could you put it together
quickly as one last contribution to webshell?
(Assignee)

Comment 6

19 years ago
Sure, I'll look at this today...
(Assignee)

Updated

19 years ago
Priority: P3 → P1
Target Milestone: M12 → M11
(Assignee)

Comment 7

19 years ago
The code was in there but ifdef'd debug in the nsWebshell.cpp and ifdef'd out in
nsApprunner.cpp.  I'm running with it enabled by default in my local build right
now.  If everything looks kosher after some testing, I'll check in my changes.

I'm adding Chris Yeh to the cc list.  Chris, what kind of behaviour do we want
when the webshell leaks are found.  The code in there right outputs a
message on the console and stops for user input before proceeding to shutdown.
If we want to hook this up to tinderbox, I'm guessing that we'll have to change
the "wait for user input" part of the behavior.

I'm upping the priority of this to P1 and moving the target milestone in to M11

Comment 8

19 years ago
cyeh is on his way out the door for vacation.
I'll offer my opinion.  We definitely can't hang up waiting
for user input on any tinderbox builds.  That defeats the
ability to build continously without user intervention
at any hour...

maybe a pref that enables this behavior, or behavior slightly modified behavior
that kicks the message to console and then exits is most effective
route.

Updated

19 years ago
Blocks: 14469

Comment 9

19 years ago
is one this ready?
(Assignee)

Comment 10

19 years ago
Yes it is.  I've enabled code and made build system changes for Windows that
check for webshell leaks on shutdown of apprunner and print the following
message to the console if any leaks are found: "XXX WARNING: Number of webshells
being leaked: 1"

Patrick has enabled this on the mac.  I need somebody who knows how
autoconf works to do three build system changes:

a) define DETECT_WEBSHELL_LEAKS globally
b) define an environment variable DETECT_WEBSHELL_LEAKS globally.
c) link in the webshell dll into apprunner inside mozilla/xpfe/bootstrap if
DETECT_WEBSHELL_LEAKS is defined in the environment.

Once the above is done, webshell leak detection should get enabled on Unix
also.  I'm ccing Ramiro who'll probably be able to help.
I'd confused as to why we need to change our linkage to make this work, but I'll
look more closely.  I think we should make the tinderboxes build with
DETECT_WEBSHELL_LEAKS and turn the tree orange if any are detected.

Updated

19 years ago
Blocks: 15160

Updated

19 years ago
No longer blocks: 14469

Updated

19 years ago
Whiteboard: what's the status here?

Comment 12

19 years ago
I can't comment on whether this has been hooked into tree status
yet but I can confirm(verify) that my Win32 commercial opt build from
1999093012 did print out:
XXX WARNING: Number of webshells
being leaked: 30
when I shut it down
(Assignee)

Comment 13

19 years ago
The status is that is enabled on mac and windows.  A Unix guy needs to make the
necessary autoconf changes to make this work on Unix when an appropriate build
flag (-enable-leak-checking or something) is used to generate the build.
(Assignee)

Updated

19 years ago
Assignee: chofmann → nisheeth
(Assignee)

Updated

19 years ago
Status: NEW → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

19 years ago
I've checked in the autoconf flag into unix.  Leaf needs to add
--enable-detect-webshell-leaks to the default unix builds.  I'll send him mail
about that.  Re-assigning this bug to myself and marking it fixed.

Updated

18 years ago
Whiteboard: what's the status here? → waiting for unix to get hooked up

Comment 15

18 years ago
I can't verifiy this until I see it in the unix builds. so as of now I'm
waiting on leaf I guess.

Updated

18 years ago
Whiteboard: waiting for unix to get hooked up → NOV-18 still waiting for unix to get hooked up

Updated

18 years ago
Status: RESOLVED → REOPENED
Target Milestone: M11 → M13

Updated

18 years ago
Assignee: nisheeth → leaf
Status: REOPENED → NEW

Updated

18 years ago
Resolution: FIXED → ---

Comment 16

18 years ago
re-opening and assigning to leaf

Comment 17

18 years ago
isn't this hooked up now? i.e . should be marked fixed?

Updated

18 years ago
Assignee: leaf → briano

Comment 18

18 years ago
You are so going to hate me for this, brian.... but i need your help.

We're being asked to turn something on in the daily release builds that
shouldn't get turned on for the *real* release builds. Do we have a way to do
this with spawnbuild and rbuild?

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 19

18 years ago
I think I have a way to do this seamlessly using my spawn_build script,
but it will require a change to the way flags get passed to rbuild.  I'll
let you know when I've got this in place.

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago18 years ago
Resolution: --- → FIXED

Comment 20

18 years ago
Okay, the necessary changes are checked in, and the test build did the
right thing, so I'm marking this fixed.  The spawn_build script now has
the ability to accept configure flags on the command line and pass them
on appropriately.  So now we can do "custom" builds using --enable-perf-metrics
and/or --enable-detect-webshell-leaks or whatever whenever we want to.

Comment 21

18 years ago
I still don't see the XXX WARNING for webshell leaks in the Linux builds as of
2000011110. Unless someone explains to me why I shouldn't see this I'm going to
have to reopen this bug.

Updated

18 years ago
Status: RESOLVED → VERIFIED

Comment 22

18 years ago
figured it out. marking VERIFIED, 2000011212 build

Updated

18 years ago
Status: VERIFIED → REOPENED

Comment 23

18 years ago
I tricked myself into verifying this bug and shouldn't have. REOPENING.
the linux builds as of 12-JAN aren't demonstrating the full test. I can see
the webshell count incrementing and decrementing in an appropriate fashion
but in the case where they aren't fully decremented at shutdown and we leak
a webshell(or several) I do NOT see the XXX Warning message. So seeing as that
is the stated original aim of this bug, I am reopening it.

Updated

18 years ago
Resolution: FIXED → ---

Comment 24

18 years ago
clearing the fixed resolution

Updated

18 years ago
Assignee: briano → travis
Status: REOPENED → NEW
Target Milestone: M13

Comment 25

18 years ago
I'm confused.  C, are you saying we see something like
the tail of this log that we get on windows, just no
XXX message. If we have a count of number of webshells
that should be good enough.  bug should now go to travis
if there is anything else that needs fixing.

not an m13 stopper. removing t milestone

! nsSecureBrowserUIImpl found: http://espn.go.com/
change lock icon to unlock ?(http 804b0002)
WEBSHELL- = 8
WEBSHELL- = 7
WEBSHELL- = 6
WEBSHELL- = 5
WEBSHELL- = 4
WEBSHELL- = 3
Hey : You are in QFA Shutdown
XXX WARNING: Number of webshells being leaked: 3
WEBSHELL- = 2

Updated

18 years ago
Target Milestone: M14

Comment 26

18 years ago
Travis, is this yours?

Comment 27

18 years ago
If this still isn't there this sounds more like a builds problem than a coding
problem.  We know the code is there and works on most platforms, just seems the
right environments aren't being set on some of the release or daily builds.

Comment 28

18 years ago
this has always been a build bug AFAIK. Remember that the XXX: Warning message predates
by a lot the console messages about incrementing and decrementing the Webshell count. That part
just never, never got hooked up on Unix.

See Niseeth's comments IN THIS BUG on 9/21, 10/01, and 10/27.

Comment 29

18 years ago
to which I question why it is assigned to me and not someone who would know
what needs to change to hook it up on the build correctly.  Leaf?

Updated

18 years ago
Assignee: travis → leaf

Comment 30

18 years ago
Yes, i'll take this puppy back.

Comment 31

18 years ago
clearing old status, where are we at now with this?

Whiteboard: NOV-18 still waiting for unix to get hooked up

Comment 32

18 years ago
Due to Beta indication in Summary, putting beta1 into keyword field.
Keywords: beta1

Updated

18 years ago
Whiteboard: [PDT-]

Updated

18 years ago
Whiteboard: [PDT-] → [PDT+]

Comment 33

18 years ago
Argh, i tried to verify that this builds on linux
(--enable-detect-webshell-leaks), and i get linker errors.
Could be a dependency problem; trying to make clean now.

Updated

18 years ago
Summary: [BETA] [Webshell] test for webshell leaking? → [Webshell] test for webshell leaking?

Comment 34

18 years ago
the beta1 stopper is that we still leak a webshell or
more on the builds where its enabled (win32).

having this assigned to leaf isn'getting us any closer to
diagnosing and fixing that leak.

travis,  how is the best person to start the investigation
about why we show one or more webshell leaks as shutdown.

Comment 35

18 years ago
If this bug is about getting the test hooked up on unix, it can probaby be done
by friday, provided i get nisheeth's help getting it to build.

If it's about actually finding the cause of the leaks, and fixing that, then it
should get reassigned.
Whiteboard: [PDT+] → [PDT+] February 11th

Comment 36

18 years ago
this bug as written was for getting the monitoring system hooked up. Unix is the last remaining platform w/o the system fully 
implemented. We leak webshells left and right in different instances, those are and should be different, specific bugs.

Comment 37

18 years ago
Here is a patch that fixed the problem with docshell. I helped Nisheeth with
this bug.


Index: Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/xpfe/bootstrap/Makefile.in,v
retrieving revision 1.78
diff -r1.78 Makefile.in
64c64
< XP_LIBS         += -lraptorwebwidget
---
> XP_LIBS         += -lraptorwebwidget -ldocshell -ljsdom

Comment 38

18 years ago
Created attachment 5182 [details] [diff] [review]
configure.in patch to turn on detect-webshell-leaks by default

Comment 39

18 years ago
Waqar's patch is checked in, getting review by cls for the turning on of
detect-webshell-leaks by default.
Status: NEW → ASSIGNED
Component: Embedding APIs → Build Config

Updated

18 years ago
Whiteboard: [PDT+] February 11th → [PDT+] I have a reviewed diff, waiting for open tree

Comment 40

18 years ago
Fixed as of 2/15/00 2:34pm; this will appear in the next verification builds
performed, and any developer's builds (unless they turn off debugging or
explicitly switch off this code)
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 41

18 years ago
I'm hanging on exit (bug 26608 ?) so i can't verify this yet

Comment 42

18 years ago
anyone know of activities likely to result in a leaked webshell on Linux? ( so i can prove this is fixed)

Comment 43

18 years ago
might be able to find some by running browser buster and then
narrowing down to specific sites.  we have found some in the past using 
this method.
(Reporter)

Comment 44

18 years ago
Is this running at the correct time?  On Solaris, it claims that a webshell is 
leaking ... and then later on after it claims that, the webshell gets released.

Comment 45

18 years ago
PDT,
A more sophisticated brand of testing may be in order here. I'm going to give it some more tries but I have not been able to leak 
a webshell and exit normally at the same time (to prove that I've leaked) so the verification here is still pending. Currently, I'm 
working with chofmann's browser buster page but I'm still open to suggestion.
Whiteboard: [PDT+] I have a reviewed diff, waiting for open tree → [PDT+] hard to verify

Comment 46

18 years ago
Maybe bruce or someone on the CC list can help verify this?

Updated

18 years ago
Whiteboard: [PDT+] hard to verify → [PDT+] need help to verify
(Reporter)

Comment 47

18 years ago
Please read my last comment on this bug.  It runs at shutdown, but appears to 
run at the _wrong_ time.  It reports a webshell leaking, but that webshell gets 
freed subsequently.

Comment 48

18 years ago
If that's the case bruce then this bugs need to be REOPENED for reexamination.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 49

18 years ago
Is this bug a total of 1 object leaked by the time we shut down? Or is the issue  
that arbitrarilly many objects will be leaked (the longer you work... the more 
you do... the more you leak)?
If this is a fixed amount of "leak," it is really equivalent to overhead bloat, 
and we can certainly live with that for beta.
...and of course if the leak tool is in error... then it is not even a bug. :-)

If this is not a growing (unbounded) leak, can we get this off the beat1 radar?

Thanks,

Jim
Whiteboard: [PDT+] need help to verify → [PDT+] [NEED INFO] need help to verify
The bug is that we haven't yet got a working system to _prevent_ us from
regressing in terms of webshell leaks.  They are traditionally a source of long
trains of leaked objects, so we've been talking for a long time about making
this one of the tinderbox tests.  We're so close...

(Reporter)

Comment 51

18 years ago
This bug is _not_ about any particular instance of a webshell leak.  What it is 
about is that for the longest time, we were continually seeing new webshell 
leaks being introduced into the tree.  They were very damaging, a source of 
regressions, and difficult to track down.  To help with this, there was a desire 
to add something so that anyone could tell when they'd introduced a new webshell 
leak.

That's what this is about.  All of the other myriad of webshell leaks need bugs 
of their own.

Comment 52

18 years ago
Finding webshell leak regressions is still a very real problem.  I've seen
numerous instances over the last few days of mozilla not exiting, or crashing on
exit, when the webshell count printed out at the end was 3 or more.  Having a
reliable webshell leak detector would make it a lot easier to track these.

Comment 53

18 years ago
jar: note this code has been up and running for Win and Mac for a long time now. It just took extra work to get implemented on 

Linux. This bug is really just, 'make the webshell test work on linux'. Given that, you can make a pdt plusness evaluation.

*Question: Are we even shipping a commercial version on linux with all the console messages still turned on?

(Reporter)

Comment 54

18 years ago
This bug has nothing to do with what we ship and whether or not it shows console 
messages.  This is something for developers to use to make sure they don't leak 
webshells.  Leaked webshells can make the product completely unusable and are a 
big nuisance for developers.  This may not need to be PDT+, but it is important 
to trying to get these webshell leaks fixed.

Please chat with some of the people who know about webshells and what's 
involved.  This isn't "just another object" leaking.
(Reporter)

Comment 55

18 years ago
My last comment wasn't intended as a flame against claudius.  I've apologized 
and explained in separate email.

Comment 56

18 years ago
Thanks for the better description... and sorry if I was missing it.
It is clear that this is a call for a fix for the leak measuring infrastructure.
That is clearly a Very Good Thing.  On the other hand, it is not (an this point)
critical for beta1.  More simply, we wouldn't hold beta1 if this bug wasn't fixed.
I'll mark the bug as going to PDT- on 3/3.  That will give you a shot at
clearing this up RSN, but we need to move it off the radar, to help focus on the
items that might hold beta.
Whiteboard: [PDT+] [NEED INFO] need help to verify → [PDT+] [NEED INFO] need help to verify w/b minus on 3/3

Comment 57

18 years ago
We passed the 3/3 date... so I'm transitioning this to PDT- for beta1, but it 
will be PDT+ for beta2.  We really want this change soon... but it is now off 
the beat1 radar.
Whiteboard: [PDT+] [NEED INFO] need help to verify w/b minus on 3/3 → [PDT-] need help to verify plus for beta2

Comment 58

18 years ago
So, to be *perfectly* clear: this bug is to track the webshell detection tool?

If so, this should be closed, because the tool is checked in. If the bug is open
because it's not built by default, then i need to reassign to nisheeth to make
the peformance problem with the test go away, so that it can be on by default in
the debug builds at least. Performance hits of 100% make the debug builds
impossible to realistically test.
Status: REOPENED → ASSIGNED

Comment 59

18 years ago
leaf: yes. this bug is for the tool.

Morever, it has been working fine on Mac and Win32. (w/o the console i can no longer verify this)
It has never worked on Linux.
It did get checked in finally for Linux but it does not work.
It was very difficult to verify and for good reason - it's not working.
Currently (03/14 builds) it's firing off at the wrong time(too early). I've seen it report 1 webshell leaked only to see the webshell 
released shortly afterward and decrement to 0.

Comment 60

18 years ago
Ok, so it's an issue with the tool itself still, reassigning to nisheeth.
Nisheeth, if you shouldn't own this bug, push back on your management so we can
find the appropriate owner.
Assignee: leaf → nisheeth
Status: ASSIGNED → NEW
(Assignee)

Comment 61

18 years ago
I have a potential fix which should prevent the error message from printing too 
early.  I've tested it on windows and am about to test it on unix.  Will check 
it in today once the tree opens.

Accepting bug and setting target milestone to M15.
Status: NEW → ASSIGNED
Target Milestone: M14 → M15
(Assignee)

Comment 62

18 years ago
The fix is checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 63

18 years ago
Finally! I apparently leaked two webshells on linux yesterday. The warning message appeared to fire off at the correct time. 
Marking verified with the 2000033009 Linux build
Status: RESOLVED → VERIFIED

Comment 64

18 years ago
Putting on beta2 keyword radar since this was "plus for beta2" during beta1 
triaging
Keywords: beta2

Updated

18 years ago
Keywords: nsbeta2
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.