411930 - Crash reporter sends wrong URL when crashing during pageload

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect, P1)

Description

[Roman R.]

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.]

Correction: it was 3.0b3pre not beta 2.

Comment 2

[(not currently active) Ted Mielczarek]

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.]

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

[(not currently active) Ted Mielczarek]

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.]

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

[(not currently active) Ted Mielczarek]

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] (inactive)]

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]

(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]

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]

(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]

> 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]

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]

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]

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]]

fixing this bug would really help me on the reproducing of crashes using the automation and to identify topcrashes etc

Comment 16

[(not currently active) Ted Mielczarek]

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)]

looking

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: 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

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

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)]

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

[(not currently active) Ted Mielczarek]

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] (inactive)]

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]

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]

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]

re-nominating

Comment 27

[Benjamin Smedberg]

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)]

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)]

new patch on the way

Comment 30

[Dietrich Ayala (:dietrich)]

Attachment: v1

Comment 31

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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]]

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)]

Dietrich, progress here?  Sorry to bug.  Just anxious!

Comment 34

[Dietrich Ayala (:dietrich)]

(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)]

(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 bugmail, email directly)]

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)]

http://hg.mozilla.org/mozilla-central/rev/8a86b4dbd991

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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]]

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]]

(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)]

ran locally and that test fails with or without the patch. investigating, there must be user-error somewhere here...

Comment 42

[Dietrich Ayala (:dietrich)]

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)]

Attachment: v2 - reduced

removes extraneous changes fixes the test failure.

/me kicks my own ass

Comment 44

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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?)]

pushed http://hg.mozilla.org/mozilla-central/rev/b64cff03aa0a

Comment 46

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cb943924422e

Comment 47

[Bob Clary [:bc] (inactive)]

Attachment: 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] (inactive)]

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)]

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)]

Filed bug 526545 for further improvements to when/where we update the URL in the crash report.

Comment 51

[Bob Clary [:bc] (inactive)]

Bug 526550