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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: timeless, Assigned: ted)

Tracking

Trunk
mozilla34
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

11 years ago
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.
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.
Component: Socorro → Breakpad Integration
Product: Webtools → Toolkit
QA Contact: socorro → breakpad.integration
Version: other → Trunk
Summary: bad filename parsing → source/header files in objdir considered to be in Hg repo if objdir is underneath srcdir
Duplicate of this bug: 512904
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.
Duplicate of this bug: 682997
Duplicate of this bug: 1039891
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.
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!
Assignee: nobody → ted
Status: NEW → ASSIGNED
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.
Attachment #8458054 - Attachment is obsolete: true
Attachment #8462611 - Flags: review?(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?
Attachment #8462611 - Flags: review?(gps) → review+
(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!
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
https://hg.mozilla.org/mozilla-central/rev/a660ebbe6ff9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1045662
Is this change live? Header files still aren't loading for me.
It is, yes. Unfortunately not all header files will work with this, generated headers will still be inaccessible for obvious reasons.
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
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
Blocks: 1076848
Split that out to bug 1076848.
Duplicate of this bug: 622555
Duplicate of this bug: 552040
You need to log in before you can comment on or make changes to this bug.