Closed Bug 462159 Opened 12 years ago Closed 6 years ago
source/header files in objdir considered to be in Hg repo if objdir is underneath srcdir
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
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.
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.
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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
Split that out to bug 1076848.
You need to log in before you can comment on or make changes to this bug.