Closed Bug 452866 Opened 16 years ago Closed 15 years ago

Teach Socorro/crash-stats about comm-central

Categories

(MailNews Core :: Build Config, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: gozer)

Details

(Whiteboard: [notacrash])

Attachments

(3 files, 5 obsolete files)

I've just been able to see some crash reports from Thunderbird since it moved from cvs to comm-central. For instance:

http://crash-stats.mozilla.com/report/index/8c2f4059-756b-11dd-9e81-001321b13766?date=2008-08-29-01

On the report, only files that are in mozilla-central have links. I'm assuming that crash-stats doesn't know about the files in comm-central and therefore doesn't show the links for them.
Confirmed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See bug 440074, where we did this for m-c.
Hm, it looks like the symbolstore script might not be handling your source files correctly. Compare:
0|1|thunderbird.exe|nsTreeSelection::RangedSelect(int,int,int)|hg:hg.mozilla.org/mozilla-central:layout/xul/base/src/tree/src/nsTreeSelection.cpp:5e208ee3e1cc|433|0xc 
vs:
0|2|thunderbird.exe|nsAbView::ReselectCards(nsIArray *,nsIAbCard *)|e:/builds/buildbot/comm-central-win32-nightly/build/mailnews/addrbook/src/nsAbView.cpp|1056|0xc 

(two frames in a row from your linked report). Note that the mailnews code doesn't have its filename mangled nicely.
(In reply to comment #3)
> Hm, it looks like the symbolstore script might not be handling your source
> files correctly. Compare:
...

So it looks like there might be two problems here:

1) Socorro needs teaching about comm-central in the same way as bug 440074
2) make buildsymbols isn't throwing out the right data for Socorro to use.

I'm going to pass this to gozer to take a look at.
Assignee: nobody → gozer
Yep, exactly. We should be able to preemptively put the comm-central bits into the server config, but that's not going to do you any good if the source files aren't coming out in the right format.

Actually, I think I see your problem, although I'm not sure how to fix it.

When we run buildsymbols, we pass $(topsrcdir) to the symbolstore.py script from the top-level (mozilla-central) Makefile.in:
http://mxr.mozilla.org/mozilla-central/source/Makefile.in#187

When we check to see if files are in a Hg repo in the script, we only check $srcdir/.hg:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#359

Since your files are in $srcdir/../, they're never getting found here. I guess we could add another else condition there, that just took the filename and walked up each directory in its pathname, looking for another .hg repo? It wouldn't be a great solution, but it'd work.
Ok, from comment 3 and comment 5 this is more a mailnews build config problem in the first instance, so moving for now but keeping gozer on the assigned in the hopes that he can look at this sometime.
Component: Socorro → Build Config
Priority: -- → P3
Product: Webtools → MailNews Core
QA Contact: socorro → build-config
bug 462159 is about a similar problem, where source/header files in the objdir wind up with source links to hgweb.
Depends on: 468965
(In reply to comment #4)
> [...]
> So it looks like there might be two problems here:
> 
> 1) Socorro needs teaching about comm-central in the same way as bug 440074

That's bug 468965 now
No longer depends on: 468965
(In reply to comment #5)
> [...]
> Since your files are in $srcdir/../, they're never getting found here. I guess
> we could add another else condition there, that just took the filename and
> walked up each directory in its pathname, looking for another .hg repo? It
> wouldn't be a great solution, but it'd work.

A better approach would be to add support for _multiple_ source directories to symbolstore,py and tell it about /foo/bar/comm-central _and_ /foo/bar/comm-central/mozilla and search them until a match for the file in question is found.
This change adds support for multiple source (-s) directories to symbolstore.py. They are searched in order, from left to right, and the first one to contain version information for a file is picked.

In this example, for comm-central, you'd specify:
  -s comm-central/mozilla -s comm-central
I thought up that exact same solution, but apparently never mentioned it here. Glad to see you sorted it out. :)
It works somewhat as you'd expect if invoked directly, I've been able to see hg.mozilla.org/comm-central references in generated .sym files.

The next thing I'd need to sort out is how to change mozilla/Makefile.in:buildsymbols so that comm-central/Makefile.in:buildsymbols could add it's own source path in there, when building comm-central.

But this patch/fix could land on it's own.
We could add an $(EXTRA_SYM_STORE_ARGS) variable to m-c's Makefile.in to let you pass arbitrary arguments in there, and just put it before all the other args on the symbolstore.py commandline. Would that work for you?
Hrm, positioning of such a variable always caused me headaches. How about a more direct solution? ${SYM_STORE_SOURCE_DIRS} ?

Pseudo-code would end up something like:
<comm-central/Makefile>
SYM_STORE_SOURCE_DIRS="comm-central/mozilla:comm-central/"

<comm-central/mozilla/Makefile>
SYM_STORE_SOURCE_DIRS="."

IFS=":"
for p in $SYM_STORE_SOURCE_DIRS; do
  SYM_STORE_SOURCE_ARGS="$SYM_STORE_SOURCE_ARGS -s $p"
done
more like $(foreach dir,$(SYM_STORE_SOURCE_DIRS),-s $dir), and SYM_STORE_SOURCE_DIRS=$topsrcdir/mozilla $topsrcdir

I think should do the trick.
Yes, for some reason, I had my shell hat on, not my make one.

So, do you want me to make such a patch and submit it for review as part of this bug or a new one?

Second question, should I keep the existing patch and this coming one separate or combined ?
Might as well do it in this bug. You're welcome to do a new patch or combine them, whatever's easiest for you.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081211 SeaMonkey/2.0a3pre - Build ID: 20081211000454

When I crash, Breakpad tells me "There was a problem sending your crash data". I think it's because Breakpad/Socorro don't know about SeaMonkey 2.0a3pre. Should I file a new bug or is it part of this one?
Socorro doesn't care about what your app thinks its version is. If your report failed it was probably a network issue.
(In reply to comment #19)
> Socorro doesn't care about what your app thinks its version is. If your report
> failed it was probably a network issue.

Hm. Recently it's every time, including a few minutes ago, and my connection was up (I didn't need to reconnect before posting these comments). -- Ah well...
Attachment #352386 - Attachment is obsolete: true
Attachment #353063 - Flags: review?
Attachment #353063 - Flags: review? → review?(ted.mielczarek)
This patch, assumes the previous one (attachment353063 [details] [diff] [review]) has already landed. It just tells buildsymbols about the other hg repos it should scan as source directories when building symbols.
Attachment #353064 - Flags: review?(ted.mielczarek)
Comment on attachment 353063 [details] [diff] [review]
Add support for multiple source directories to buildsymbols

+SYM_STORE_SOURCE_ARGS = $(foreach dir,$(SYM_STORE_SOURCE_DIRS),-s $(dir))
+

Just put the $(foreach) inline in the rule commands. You can put it on its own line if it gets too long.

+def GetVCSFilename(file, srcdirs):
     """Given a full path to a file, and the top source directory,

Modify the comment here to mention multiple source dirs, please.

+        for srcdir in srcdirs:
+            if os.path.isdir(os.path.join(path, "CVS")):
+                fileInfo = CVSFileInfo(file, srcdir)
+                if fileInfo:
+                   root = fileInfo.root

Can you break out of the loop early after the if blocks if fileInfo is set, or do you rely on it finding things in the last srcdir in the list?

         if srcdir is not None:
-            self.srcdir = os.path.normpath(srcdir)
+            self.srcdir = [os.path.normpath(a) for a in srcdir]

With optparse and the "append" action, does srcdir wind up as None or just [] if it's not specified?

Also, can you change the member variable name to |self.srcdirs| to be clearer?

I think there are a few other comments in the file that didn't make it into your patch context that mention srcdir. Please fix those up as well.

Looks like you'll break this Solaris-only case, please try to fix it (although it's ok if you can't test it):
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#513
Attachment #353063 - Flags: review?(ted.mielczarek) → review+
Attachment #353064 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 353064 [details] [diff] [review]
Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds

$(topsrcdir)/mozilla $(topsrcdir) " $@
 
If that extra space at the end of the quoted string is not necessary, please remove it.
Address comment 24
Attachment #353064 - Attachment is obsolete: true
Attachment #353524 - Flags: review?(ted.mielczarek)
(In reply to comment #23)
> (From update of attachment 353063 [details] [diff] [review])
> +SYM_STORE_SOURCE_ARGS = $(foreach dir,$(SYM_STORE_SOURCE_DIRS),-s $(dir))
> +
> 
> Just put the $(foreach) inline in the rule commands. You can put it on its own
> line if it gets too long.

Fine, what's the standard for aligning the '\' character in continued lines ?

> +def GetVCSFilename(file, srcdirs):
>      """Given a full path to a file, and the top source directory,
> 
> Modify the comment here to mention multiple source dirs, please.
> 
> +        for srcdir in srcdirs:
> +            if os.path.isdir(os.path.join(path, "CVS")):
> +                fileInfo = CVSFileInfo(file, srcdir)
> +                if fileInfo:
> +                   root = fileInfo.root
> 
> Can you break out of the loop early after the if blocks if fileInfo is set, or
> do you rely on it finding things in the last srcdir in the list?

Nope, we stop at the first place we find something, but you had to follow the
if/elseif chain to the end to see the break:

 if fileInfo:
  vcsFileInfoCache[file] = fileInfo
  break

>          if srcdir is not None:
> -            self.srcdir = os.path.normpath(srcdir)
> +            self.srcdir = [os.path.normpath(a) for a in srcdir]
> 
> With optparse and the "append" action, does srcdir wind up as None or just []
> if it's not specified?

It ends up None, unless specified default=[]

> Also, can you change the member variable name to |self.srcdirs| to be clearer?

Yes, that was part of the carefull, change as little as possible, process of implementing this. Renamed.

> I think there are a few other comments in the file that didn't make it into
> your patch context that mention srcdir. Please fix those up as well.

I'll look for those.

> Looks like you'll break this Solaris-only case, please try to fix it (although
> it's ok if you can't test it):
> http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#513

Interesting, for some reason, it gets passed in full path names as opposed to relative ones ? Easily fixed.
(In reply to comment #26)
> Fine, what's the standard for aligning the '\' character in continued lines ?

I like them lined up out to whatever the longest line is.

> Nope, we stop at the first place we find something, but you had to follow the
> if/elseif chain to the end to see the break:

Oops, missed that.

> Interesting, for some reason, it gets passed in full path names as opposed to
> relative ones ? Easily fixed.

Yeah, I dunno, Solaris is weird.
Comment on attachment 353524 [details] [diff] [review]
Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds v2

FWIW, you don't need to request review if I r+ed with comments. Just carry forward the review when you fix what I asked for.
Attachment #353524 - Flags: review?(ted.mielczarek) → review+
Update to latest comm-central status
Attachment #353524 - Attachment is obsolete: true
Added all recommended changes.
Attachment #353063 - Attachment is obsolete: true
Attachment #360935 - Flags: review?(ted.mielczarek)
Comment on attachment 360935 [details] [diff] [review]
Add support for multiple source directories to buildsymbols v2

                             if sys.platform == "sunos5":
-                                start = filename.find(self.srcdir)
+                                start = filename.find(self.srcdirs)

You're going to need to fix this somehow. You could just hack it to be self.srcdirs[0] or something (but check  |if self.srcdirs:| first?)

-                      action="store", dest="srcdir",
+                      action="append", dest="srcdir", default=[],

Make that dest="srcdirs", and change the one reference to options.srcdir below to options.srcdirs.
Attachment #360935 - Flags: review?(ted.mielczarek) → review+
This fixes the Solaris-specific code, but I am not entirely certain what it was for in the first place.

The only thing it would ever do is strip a mysterious prefix from the source files. i.e.

if 

srcdir=/home/gozer/mozilla-central
filename=/home/gozer/mozilla-central/somefile

The only time it would do something is if at one point

filename=/mysterious/prefix/home/gozer/mozilla-central/somefile

and would strip the /mysterious/prefix part.

In any case, this patch preserves that behaviour across multiple source directories.
Attachment #360935 - Attachment is obsolete: true
Attachment #361996 - Flags: review?(ted.mielczarek)
Just for reference, the special sunos case I am talking about in comment #32 was introduced by bug 391361.
Attachment #361996 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 361996 [details] [diff] [review]
[checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch)

Looks good, thanks.
Attachment 361996 [details] [diff] needs checkin-in in mozilla-central/mozilla-1.9.1 before attachment 360934 [details] [diff] [review] can land in comm-central.
Keywords: checkin-needed
Comment on attachment 361996 [details] [diff] [review]
[checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch)

Does not apply to mozilla-central:
{
patching file toolkit/crashreporter/tools/symbolstore.py
Hunk #3 FAILED at 348
Hunk #6 FAILED at 510
2 out of 8 hunks FAILED
}
Original patch was based on mozilla-1.9.1, rebased to mozilla-central. Mostly whitespace changes.
Attachment #361996 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 362667 [details] [diff] [review]
[checked in] Add support for multiple source directories to buildsymbols v4 (mozilla-central)

Checked in: http://hg.mozilla.org/mozilla-central/rev/d2d0110cd69a
Attachment #362667 - Attachment description: Add support for multiple source directories to buildsymbols v4 (mozilla-central) → [checked in] Add support for multiple source directories to buildsymbols v4 (mozilla-central)
This is now in on mozilla-central, lets give it a few hours before pushing to 1.9.1.
Keywords: checkin-needed
(In reply to comment #39)
> This is now in on mozilla-central, lets give it a few hours before pushing to
> 1.9.1.

Just realized this didn't make it to 1.9.1 yet, that's unfortunate, as I just tagged for TB Beta 2 RC1. Can we try and get this landed soon, so we might catch it if/when we do RC2 ?
Attachment #361996 - Attachment description: Add support for multiple source directories to buildsymbols v3 → Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch)
Attachment #361996 - Attachment is obsolete: false
Comment on attachment 361996 [details] [diff] [review]
[checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch)

Un-obsoleting as this is required for mozilla-1.9.1
Comment on attachment 361996 [details] [diff] [review]
[checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch)

Checked into 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ed07e0df2ec
Attachment #361996 - Attachment description: Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch) → [checked in] Add support for multiple source directories to buildsymbols v3 (mozilla-1.9.1 patch)
Comment on attachment 360934 [details] [diff] [review]
[checked in] Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds v3

Checked in: http://hg.mozilla.org/comm-central/rev/ce0e94b8b433
Attachment #360934 - Attachment description: Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds v3 → [checked in] Tell buildsymbols about comm-central & mozilla-central for mozilla-central builds v3
This should be fixed now.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(In reply to comment #42)
> (From update of attachment 361996 [details] [diff] [review])
> Checked into 1.9.1:
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ed07e0df2ec

A little disingenuous, as this is in fact part of the build. (Not that it gets built into Firefox, but we certainly use it.)
Fresh off the press:

http://crash-stats.mozilla.com/report/index/58b6007e-0e56-4cc2-8718-200fa2090219?p=1

We've got links for comm-central now :-) but the ones for mozilla-1.9.1 are missing, although most of the backend data seems to be there.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I filed bug 479055 on that yesterday.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Whiteboard: [notacrash]
You need to log in before you can comment on or make changes to this bug.