Last Comment Bug 411930 - Crash reporter sends wrong URL when crashing during pageload
: Crash reporter sends wrong URL when crashing during pageload
Status: RESOLVED FIXED
[sg:want P3][crashkill][crashkill-met...
:
Product: Toolkit
Classification: Components
Component: Breakpad Integration (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9.3a1
Assigned To: Dietrich Ayala (:dietrich)
:
Mentors:
Depends on:
Blocks: 526545 548208
  Show dependency treegraph
 
Reported: 2008-01-11 09:31 PST by Roman R.
Modified: 2011-04-14 18:19 PDT (History)
20 users (show)
mbeltzner: blocking1.9.2+
benjamin: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta2-fixed
wontfix


Attachments
wip (15.55 KB, patch)
2009-09-29 12:25 PDT, Dietrich Ayala (:dietrich)
no flags Details | Diff | Splinter Review
v1 (15.26 KB, patch)
2009-10-21 14:50 PDT, Dietrich Ayala (:dietrich)
paul: review+
Details | Diff | Splinter Review
v2 - reduced (800 bytes, patch)
2009-10-29 17:11 PDT, Dietrich Ayala (:dietrich)
paul: review+
Details | Diff | Splinter Review
testcase (262 bytes, text/html)
2009-11-04 08:47 PST, Bob Clary [:bc:]
no flags Details

Description Roman R. 2008-01-11 09:31:02 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008011005 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008011005 Minefield/3.0b3pre

I just had FF 3 beta 2 crash, and the crash reporter got launched. I looked at the report and saw that the URL it was going to report was the URL of an active tab, which was just a simple page showing contents of a local file. I don't think the problem was on that page. FF was downloading 3 large files at the time. Since FF is a multipage browser, ALL URLs need to be recorded if the exact one associated with the problem can't be identified.

Reproducible: Didn't try
Comment 1 Roman R. 2008-01-11 09:32:48 PST
Correction: it was 3.0b3pre not beta 2.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2008-01-11 11:44:40 PST
We WONTFIX'ed the idea of sending the URLs from all tabs over in bug 401961.  I thought there was a bug filed on getting more accurate URLs, since sometimes it doesn't get the URL if you crash during load, but it's still not going to help if a background tab crashes.
Comment 3 Roman R. 2008-01-11 14:03:19 PST
Well, my report seems to duplicate the one you mentioned, Ted. Aside from the URL, does the crash tracker send data useful in troubleshooting crashes? Is URL crucial in this process?

Is there a bug report asking for being able to edit what's being sent? I saw what was about to be sent (if it was going to work) and wanted to edit out my profile name. Being able to view and agree or cancel isn't enough.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2008-01-11 14:07:41 PST
The URL isn't crucial, but it is often helpful.  I'm adding a comment field to the client over in bug 404855, so that would at least allow you to indicate the URL that actually crashed, if you think you know which one it is.  I'd like to avoid letting users edit the URL, since I don't think it adds a lot of value, but it will add UI complexity.
Comment 5 Roman R. 2008-01-11 14:16:07 PST
That's good. I don't have time to read all comments in other bug, so I will say here: it would be useful to (wouldn't it?) to provide additional notes as it was possible with the Talkback.

Thanks for listening to our feedback! At least, somebody remembers that Firefox is for users not just developers.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2009-08-14 08:56:34 PDT
I wouldn't block on this. It would be nice to make it more accurate, but for topcrashes it seems pretty likely that with a huge number of reports we'll get something usable.
Comment 7 Bob Clary [:bc:] 2009-08-14 09:19:40 PDT
Ted, the crash density for the current urls in the most recent test is on the order of 1 in 600 on Mac and 1 in 3000 for windows and predominately we are finding Flash crashes now. We do manage to get some useful results for Firefox, but the situation is marginal at best.
Comment 8 cmtalbert 2009-08-17 13:15:17 PDT
(In reply to comment #6)
> I wouldn't block on this. It would be nice to make it more accurate, but for
> topcrashes it seems pretty likely that with a huge number of reports we'll get
> something usable.

Ted, your assumption here does not stand up to the real data. The data is showing that we are reproducing less than 1% of what should be reproducible crashes (this is after we get rid of everything with a login and flash). I don't think that we're making up for this in sheer volume.  We really need this to be fixed on  a *shipping* release so that we can get accurate data once those go into the field.  I'm re-nominating for blocking 1.9.2.
Comment 9 Benjamin Smedberg [:bsmedberg] 2009-08-17 13:26:24 PDT
What is the proposed resolution to this bug? Pick a better single URL to send? If so, how would you do that? We really don't know which page is crashing in most cases, so we send the topmost tab or the one most-recently-loaded or something like that.

If you're proposing that we send *all* the URLs loaded in every window/tab, I don't think that's a good idea from a privacy perspective.
Comment 10 cmtalbert 2009-08-17 13:47:34 PDT
(In reply to comment #9)
> What is the proposed resolution to this bug? Pick a better single URL to send?
> If so, how would you do that? We really don't know which page is crashing in
> most cases, so we send the topmost tab or the one most-recently-loaded or
> something like that.
We were hoping there was a way that you could write the URL that the user is about to load into the session store cache (or wherever you pull it from) *before* the real pageload begins.  Then once the page loads and crashes, when you pull the "last page viewed" you'd have that URL instead of the previous URL.  Is that not possible?  It sounds deceptively easy, but that may be because we don't know the implementation here.

> 
> If you're proposing that we send *all* the URLs loaded in every window/tab, I
> don't think that's a good idea from a privacy perspective.
No, no we're not suggesting that.  Not only is that bad for privacy, that introduces even more noise into the data and we definitely don't want that.
Comment 11 Benjamin Smedberg [:bsmedberg] 2009-08-17 17:17:25 PDT
> We were hoping there was a way that you could write the URL that the user is
> about to load into the session store cache (or wherever you pull it from)

You probably could. You probably could also set the crash reporter URL when you select a tab, so that the URL is the URL of the currently selected tab instead of the one that finished loading last. I expect that will give you much better results also.

Of course you'd be trading crashes-on-unload for crashes-on-load, but I'm willing to believe that's a reasonable tradeoff.

In any case we wouldn't stop-ship a release for this.

cc'ing dietrich who owns session-restore, since we're getting the data from there
Comment 12 chris hofmann 2009-08-17 17:44:23 PDT
yes, that's what we are looking for in the first version of the automated crash testing system -->  crashes on load.   

Thats what we are able to do now with pretty good success when the stars align.  tomcat, bc, and others were able to find a ton of crashes using the data that we have now, but there is still to be found if we can get them more reliable crash on load sites.

if there is a flag for "really, really wanted 1.9.2" then this bug ought to get it.
Comment 13 chris hofmann 2009-08-21 13:44:16 PDT
I guess any increase in the frequency of saving might have performance costs, and it appears from https://bugzilla.mozilla.org/show_bug.cgi?id=511964 and http://lifehacker.com/5342636/how-to-fix-annoying-youtube-jumpiness-in-firefox these costs are noticeable.

maybe there is a way to optimize both the way we save the urls and improve the accuracy of knowing which url was loaded last.  lets hope so..
Comment 14 Benjamin Smedberg [:bsmedberg] 2009-08-25 09:42:51 PDT
We don't need to save the data to disk, just call the crashreporter function which saves it in memory... the fact that the session-restore code is doing the work doesn't mean it actually needs to save the session-restore file to disk.
Comment 15 Carsten Book [:Tomcat] - PTO-back Sept 4th 2009-09-28 15:40:08 PDT
fixing this bug would really help me on the reproducing of crashes using the automation and to identify topcrashes etc
Comment 16 Ted Mielczarek [:ted.mielczarek] 2009-09-29 04:19:07 PDT
Can we get some toolkit or browser expert help here? The implementation of this is in browser code:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2723

It's currently being called in onTabLoad:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#819

and onTabSelect:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#861

The latter seems fine, I think the former is probably not quite right. We should be setting the URL when the user starts a load in a tab, whether that's by inputting a URL or clicking a link or whatever. I'm sure there's an observer for that somewhere, but I don't know where.

This still isn't going to get us 100% accuracy, since crashing in background tabs will give you the wrong URL, but it's the best we can do, I think.
Comment 17 Dietrich Ayala (:dietrich) 2009-09-29 08:47:32 PDT
looking
Comment 18 Dietrich Ayala (:dietrich) 2009-09-29 12:25:59 PDT
Created attachment 403549 [details] [diff] [review]
wip

update url when we receive TabOpen. haven't tested to see if this actually works.

(also, move a bunch of services to cached lazy getters)
Comment 19 Ted Mielczarek [:ted.mielczarek] 2009-09-29 13:51:30 PDT
You know sdwilsh added helpers to XPCOMUtils.jsm to do lazy getters, right?
https://developer.mozilla.org/en/XPCOMUtils.jsm#defineLazyServiceGetter%28%29
Comment 20 Ted Mielczarek [:ted.mielczarek] 2009-09-29 13:55:53 PDT
You should be able to test by creating a page that crashes on pageload (might have to hack something to cause a crash), and then click the "details" button on the crash reporter to verify that you have the right URL in there. Hard to test in an automated fashion right now, unfortunately.
Comment 21 Dietrich Ayala (:dietrich) 2009-10-02 11:37:06 PDT
hm, can't get a crash reporter to pop up on win7 trunk debug build. any special trickery needed to get that to happen?
Comment 22 Ted Mielczarek [:ted.mielczarek] 2009-10-02 11:54:03 PDT
If you didn't build with MOZILLA_OFFICIAL=1 in your mozconfig, you'll need to set MOZ_CRASHREPORTER=1 in the environment before running your build.
Comment 23 Bob Clary [:bc:] 2009-10-06 11:36:29 PDT
I get on start up:

nsSessionStore could not be initialized: TypeError: Cc['@mozilla.org/browser/sessionstore;1'] is undefined

This wasn't a start up/onload crash, but I got

bp-788a7eb7-cdc1-4241-b197-829132091006
bp-a15fee94-dbec-4946-a4e8-b310b2091006

I didn't see any url information in the details.
Comment 24 chris hofmann 2009-10-21 08:34:59 PDT
increased need to get better at tracking down crashes hopefully means increased importance and priority for this bug.  

any updates on where the patch is?

it would really help if this could make betas and rc's on 3.5.x and the 3.6 train.

this could have a real impact in getting to reproducable test cases of sites that are crashing.
Comment 25 chris hofmann 2009-10-21 08:42:02 PDT
risk on the patch should be relativity easy to assess, and isloated to mostly to performance concerns when loading lots of pages and getting the right history update to happen.

we really need this to go to the next level on automated crash testing.
Comment 26 chris hofmann 2009-10-21 08:44:48 PDT
re-nominating
Comment 27 Benjamin Smedberg [:bsmedberg] 2009-10-21 09:26:00 PDT
Still not going to block a release on this, though I'd take a sufficiently simple patch: messing with session restore is not going to be risk-free.
Comment 28 Damon Sicore (:damons) 2009-10-21 11:14:52 PDT
I'm not sure that we shouldn't block 1.9.2 for this, if we are serious about fixing crashes overall.
Comment 29 Dietrich Ayala (:dietrich) 2009-10-21 13:25:39 PDT
new patch on the way
Comment 30 Dietrich Ayala (:dietrich) 2009-10-21 14:50:00 PDT
Created attachment 407626 [details] [diff] [review]
v1
Comment 31 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-10-22 12:07:53 PDT
Comment on attachment 407626 [details] [diff] [review]
v1

>@@ -734,16 +767,18 @@ SessionStoreService.prototype = {
>     aBrowser.addEventListener("change", this, true);
>     aBrowser.addEventListener("input", this, true);
>     aBrowser.addEventListener("DOMAutoComplete", this, true);
>     aBrowser.addEventListener("scroll", this, true);
>     
>     if (!aNoNotification) {
>       this.saveStateDelayed(aWindow);
>     }
>+
>+    this._updateCrashReportURL(aWindow);
>   },
...
>   /**
>    * Annotate a breakpad crash report with the currently selected tab's URL.
>    */
>   _updateCrashReportURL: function sss_updateCrashReportURL(aWindow) {
>     if (!Ci.nsICrashReporter) {
>       // if breakpad isn't built, don't bother next time at all
>@@ -2729,18 +2747,17 @@ SessionStoreService.prototype = {
>     try {
>       var currentURI = aWindow.gBrowser.currentURI.clone();
>       // if the current URI contains a username/password, remove it
>       try { 
>         currentURI.userPass = ""; 
>       } 
>       catch (ex) { } // ignore failures on about: URIs
> 
>-      var cr = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsICrashReporter);
>-      cr.annotateCrashReport("URL", currentURI.spec);
>+      CrashReporter.annotateCrashReport("URL", currentURI.spec);
>     }
>     catch (ex) {
>       // don't make noise when crashreporter is built but not enabled
>       if (ex.result != Components.results.NS_ERROR_NOT_INITIALIZED)
>         debug(ex);
>     }
>   },
> 

So when _updateCrashURL is called from onAddTab above, I don't think currentURI is going to be what you're expecting. Correct me if I'm wrong, but isn't currentURI always going to be about:blank? (I did limited testing with some other work that listens for TabOpen)
Comment 32 Mike Beltzner [:beltzner, not reading bugmail] 2009-10-26 08:13:15 PDT
I agree with Damon; we need to improve our visibility into things that are causing problems for our users, and if we have a deep suspicion (as per comment 8) that the data we're currently receiving is of little use, then we need to take steps to address, and I don't see us having the confidence to do this in a security and stability update.

I'm marking this as a P1 since I think we want it in nightlies sooner rather than later. Dietrich, feel free to ask around for help if you need it.
Comment 33 Damon Sicore (:damons) 2009-10-26 16:41:23 PDT
Dietrich, progress here?  Sorry to bug.  Just anxious!
Comment 34 Dietrich Ayala (:dietrich) 2009-10-26 18:18:54 PDT
(In reply to comment #31)
> So when _updateCrashURL is called from onAddTab above, I don't think currentURI
> is going to be what you're expecting. Correct me if I'm wrong, but isn't
> currentURI always going to be about:blank? (I did limited testing with some
> other work that listens for TabOpen)

wfm when opening a new tab from an existing tab, or a link from an external application. the only time i see about:blank is when opening a new blank tab, which is expected.
Comment 35 Dietrich Ayala (:dietrich) 2009-10-26 18:28:08 PDT
(In reply to comment #33)
> Dietrich, progress here?  Sorry to bug.  Just anxious!

from the testing i've done, the patch should improve accuracy in those scenarios (opened from existing tab, opened from external app).
Comment 36 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-10-27 11:01:08 PDT
Comment on attachment 407626 [details] [diff] [review]
v1

If it's working in real life, that's much better than my hypothetical  testing.

r=zpao
Comment 37 Dietrich Ayala (:dietrich) 2009-10-28 14:34:05 PDT
http://hg.mozilla.org/mozilla-central/rev/8a86b4dbd991
Comment 38 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-10-28 18:54:43 PDT
Looks like this caused a unit test failure:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_339445.js | sessionStorage value has been duplicated - Got PENDING, expected SUCCESS

I'll probably back it out soon unless I hear otherwise.
Comment 39 Daniel Holbert [:dholbert] 2009-10-28 21:11:19 PDT
dietrich backed this out as:
http://hg.mozilla.org/mozilla-central/rev/75e7a30cf385

On one of the affected tinderbox cycles, there was also an xpcshell storage test failure.  This isn't a known-sporadic test, AFAICT, so I'm assuming it's connected to this bug's landing.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1256783108.1256785108.16178.gz
{
TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\mozilla-central-win32-opt-unittest-everythingelse\build\xpcshell\tests\test_storage\unit\test_storage_fulltextindex.js | test failed (with xpcshell return code: 1), see following log:
}
(I'm not pasting the "see following log" context here, since it's ~20 lines long and I can't immediately tell which chunk of it caused the test failure. The link to it the failure log is above, though.)
Comment 40 Daniel Holbert [:dholbert] 2009-10-28 21:13:55 PDT
(Oh, sorry -- this is a _sessionStorage_ bug, whereas the additional test failure I noted in comment 39 is in _storage_ code. So, that one's probably unrelated to this checkin. I'll file an [orange] bug on it.)
Comment 41 Dietrich Ayala (:dietrich) 2009-10-28 22:57:36 PDT
ran locally and that test fails with or without the patch. investigating, there must be user-error somewhere here...
Comment 42 Dietrich Ayala (:dietrich) 2009-10-29 14:34:01 PDT
duplicateTab() call is failing for all tests for me locally, because aWindow.ownerDocument.defaultView.__SSi does not exist (not tracking the window?!).
Comment 43 Dietrich Ayala (:dietrich) 2009-10-29 17:11:01 PDT
Created attachment 409231 [details] [diff] [review]
v2 - reduced

removes extraneous changes fixes the test failure.

/me kicks my own ass
Comment 44 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-10-29 17:47:01 PDT
Comment on attachment 409231 [details] [diff] [review]
v2 - reduced

I was hoping to slip those other changes into 1.9.2 too for a potential perf win. Oh well. They'll get made when somebody goes through and cleans up the sessionstore.
Comment 45 Brad Lassey [:blassey] (use needinfo?) 2009-10-30 11:13:32 PDT
pushed http://hg.mozilla.org/mozilla-central/rev/b64cff03aa0a
Comment 47 Bob Clary [:bc:] 2009-11-04 08:47:34 PST
Created attachment 410259 [details]
testcase

modified testcase from https://bugzilla.mozilla.org/show_bug.cgi?id=503237 to crash on load.

On the trunk this crashes immediately. I don't see the url in my profile except in a cache file. When I restart the profile, it crashes immediately without giving me the session restore option of selecting which windows/tabs to restore.
Comment 48 Bob Clary [:bc:] 2009-11-04 09:24:38 PST
more data: testing with mac. New profiles with show home page or restore windows/tabs will show the restore session window after the crash. On the first restart from the crash the list of urls does not show the crashing url, but on subqsequent crashes it does. Tomcat and I can both see this behavior. I'm not sure why my default profile won't show the session restore after the crash. It has restore windows/tabs set and has a couple of dozen tabs open. You can find all my crashes from today with bc: in the comment + the url I was testing.
Comment 49 Dietrich Ayala (:dietrich) 2009-11-04 10:12:22 PST
Bob, can you file a followup bug for the restoration problem? Let's separate that out from the problem of showing the proper crash URL.
Comment 50 Dietrich Ayala (:dietrich) 2009-11-04 11:01:55 PST
Filed bug 526545 for further improvements to when/where we update the URL in the crash report.
Comment 51 Bob Clary [:bc:] 2009-11-04 11:06:03 PST
Bug 526550

Note You need to log in before you can comment on or make changes to this bug.