462159 - source/header files in objdir considered to be in Hg repo if objdir is underneath srcdir

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[timeless]

Frame 	Module 	Signature 	Source
0 	xul.dll 	CallQueryInterface<nsIDOMHTMLElement,nsIDOMElement> 	obj-firefox/dist/include/xpcom/nsISupportsUtils.h:203
1 	xul.dll 	nsCoreUtils::GetDOMElementFor 	accessible/src/base/nsCoreUtils.cpp:171

[obj-firefox/dist/include/xpcom/nsISupportsUtils.h:203]
leads to
http://hg.mozilla.org/mozilla-central/annotate/5bd6876be7f2/obj-firefox/dist/include/xpcom/nsISupportsUtils.h#l203

which isn't happy. not certain whose fault this is.

Comment 1

[(not currently active) Ted Mielczarek]

I thought I commented here, but I must have lost it.
From the raw dump:
0|0|xul.dll|CallQueryInterface<nsIDOMHTMLElement,nsIDOMElement>(nsIDOMHTMLElement *,nsIDOMElement * *)|hg:hg.mozilla.org/mozilla-central:obj-firefox/dist/include/xpcom/nsISupportsUtils.h:5bd6876be7f2|203|0x8
This is a bug in symbolstore.py. If your objdir is underneath the srcdir, files in the objdir will be considered to be in the repository. We don't actually check each file to see if it's in the hg repo, we just look to see if the file is underneath the srcdir:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#359

To fix this we'd have to actually ask Hg if each file was in the repository.

Comment 3

[(not currently active) Ted Mielczarek]

From the dupe:
   Description  From  Jesse Ruderman   2009-08-27 00:26:18 PDT   (-) [reply]

http://crash-stats.mozilla.com/report/index/babc805e-0760-4088-90e4-1a36d2090827

A few of the source links are fine, but two are incorrect:

obj-firefox/xpcom/build/nsCOMPtr.cpp:81
obj-firefox/dist/include/xpcom/nsISupportsImpl.h:197

Please make these link to the correct files in the source tree, even if it
requires some hackery.  Having some of the source links not work limits their
usefulness.

------- Comment #1 From Ted Mielczarek (:luser) 2009-08-27 08:49:36 PDT (-) [reply] -------

It's...difficult. Those files really do get compiled from the objdir, so
they're not part of the source repo at that point. Mapping them back to the
original file is hard, and I don't know a way to easily do it.

------- Comment #2 From Jesse Ruderman 2009-08-27 09:03:43 PDT (-) [reply] -------

You could keep a list of .h files and their locations in the tree.

Could this be fixed in the build process?  Either by not copying the files, or
by post-processing the symbols?

------- Comment #3 From Ted Mielczarek (:luser) 2009-08-27 09:12:23 PDT (-) [reply] -------

Well, we wouldn't copy the files if there wasn't a reason. It's possible we
could either stick #line directives into files when we copy them, to point at
the original source location, or post-process the symbols somehow.

Comment 6

[(not currently active) Ted Mielczarek]

I think this is more feasible to fix nowadays. What we need is a registry of where each header file in the objdir came from, and we have that nowadays--install manifests. We ought to be able to read the dist/include install manifest and check each file to see if it was installed that way. The install manifest knows the source location, so we can swap that out.

The only thing this still won't fix is generated sources, since we don't actually have a location for them. We'd have to upload the generated source somewhere for that to work.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: Use install manifests to track header files from dist/include back to srcdir in symbolstore.py

This works for me. I need to write some unit tests for it. Nice side-effect of moving things to moz.build with a nice Python backend!

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: Use install manifests to track header files from dist/include back to srcdir in symbolstore.py

I've had this sitting around for a week, but a bugzilla bug prevents me from using bzexport to attach patches to Toolkit bugs.

This needs a run past Try to make sure it works on other platforms.

I found a minor bug in InstallManifest error handling, that's rolled into this patch.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8462611 [details] [diff] [review]
Use install manifests to track header files from dist/include back to srcdir in symbolstore.py

Review of attachment 8462611 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix at least "Foo" before landing.

The secondary and unintended benefits of a unified build config via moz.build files continue to surprise me.

::: toolkit/crashreporter/tools/symbolstore.py
@@ +289,5 @@
> +    args = []
> +    for arg in install_manifest_args:
> +        bits = arg.split(',')
> +        if len(bits) != 2:
> +            raise "Foo"

raise "Foo"?

::: toolkit/crashreporter/tools/unit-symbolstore.py
@@ +367,5 @@
> +        self.symboldir = os.path.join(self.test_dir, 'symbols')
> +        os.mkdir(self.symboldir)
> +
> +    @patch("subprocess.Popen")
> +    def testFileMapping(self, mock_Popen):

I didn't really grok all that's happening in this test. But from what I can gather from the rest of the patch, it seems sane.

That being said, I'm not a huge fan of the testing approach. You are testing manifest processing standalone and then testing file mapping standalone. I would much rather see an integration test that actually consumes an install manifest. But it looks like that may be a bit difficult to implement. You have good test coverage over install manifest processing, so I'm not super worried.

@@ +393,5 @@
> +                    [
> +                        'PUBLIC xyz 123\n'
> +                    ]
> +                )
> +            mock_Popen.return_value.stdout = mk_output(dumped_files)

Why do you assign this in a loop? It isn't accessed until d.Process() or d.Finish(), correct? If you move it, I guess mk_output() can be inlined?

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to Gregory Szorc [:gps] from comment #9)
> raise "Foo"?

Oops, missed that. Thanks!

> That being said, I'm not a huge fan of the testing approach. You are testing
> manifest processing standalone and then testing file mapping standalone. I
> would much rather see an integration test that actually consumes an install
> manifest. But it looks like that may be a bit difficult to implement. You
> have good test coverage over install manifest processing, so I'm not super
> worried.

Writing tests for this script is kind of a PITA (as you may surmise). I could mash both tests into one, but I figured this way got me all the test coverage I wanted without having to write a mega-test.

> > +            mock_Popen.return_value.stdout = mk_output(dumped_files)
> 
> Why do you assign this in a loop? It isn't accessed until d.Process() or
> d.Finish(), correct? If you move it, I guess mk_output() can be inlined?

Uh, that was completely accidental. I must have indented that block incorrectly. It only needs to be run once. I do use mk_output twice though, for stdout here and for expected_output below.

Thanks for the review!

Comment 11

[(not currently active) Ted Mielczarek]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=378554e418f9

Comment 12

[(not currently active) Ted Mielczarek]

The tests failed on Windows (as I suspected they might), and apparently this broke on B2G as well. Pretty trivial fixes for both, but sanity checking:
https://tbpl.mozilla.org/?tree=Try&rev=2e867308ece8

Comment 13

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a660ebbe6ff9

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a660ebbe6ff9

Comment 15

[(Away)]

Is this change live? Header files still aren't loading for me.

Comment 16

[(not currently active) Ted Mielczarek]

It is, yes. Unfortunately not all header files will work with this, generated headers will still be inaccessible for obvious reasons.

Comment 17

[(not currently active) Ted Mielczarek]

If you look at a symbol file from yesterday's nightly you can see:
http://symbols.mozilla.org/firefox/firefox.pdb/C75EDDEAFB874537A034DB1A380D33062/firefox.sym

has:
FILE 5 hg:hg.mozilla.org/mozilla-central:xpcom/base/nsError.h:6a63bcb6e0d3

There are a few other things that still won't work properly, such as NSPR headers:
FILE 394 hg:hg.mozilla.org/mozilla-central:obj-firefox/dist/include/nspr/prinrval.h:6a63bcb6e0d3

Comment 18

[(Away)]

Looking at the first October nightly: http://symbols.mozilla.org/firefox/xul.pdb/28D210BA209049E2B10F488F7C4963812/xul.sym

nsTArray is my usual example since it's so common:
FILE 477 hg:hg.mozilla.org/mozilla-central:xpcom/glue/nsTArray.h:14665b1de5ee

I can pull it up manually in a browser:
http://hg.mozilla.org/mozilla-central/raw-file/14665b1de5ee/xpcom/glue/nsTArray.h

But the debugger can't. With |!sym noisy| I get this warning:
SRCSRV:  c:\builds\moz2_slave\m-cen-w32-ntly-000000000000000\build\obj-firefox\dist\include\nstarray.h not indexed

Comment 19

[(not currently active) Ted Mielczarek]

Split that out to bug 1076848.