964349 - Move CrashReporter.annotateCrashReport() calls out of sessionstore

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Bug 375083 introduced CrashReporter.annotateCrashReport() calls to sessionstore to annotate crash reports with the right URLs. I have no clue why it was decided to do that in sessionstore, I think tabbrowser would be the right place for this.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-964349-Move-CrashReporter.annotateCrashReport-ca.patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

nit: you can use Services.appinfo.annotateCrashReport

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Hmm, I also just found out that we're doing the same in browser.js already:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3998

This covers load events but also does it for subframes and background tabs, which is kind of weird, no?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-964349-Move-CrashReporter.annotateCrashReport-ca.patch, v2

(In reply to :Gavin Sharp (email [e-mail]) from comment #2)
> nit: you can use Services.appinfo.annotateCrashReport

Done.

(In reply to Tim Taubert [:ttaubert] from comment #3)
> Hmm, I also just found out that we're doing the same in browser.js already:

This was added by bug 526545. I removed this and let it be handled by the tabbrowser as well by always updating the crash report URL, not just for STATE_STOP.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8366137 [details] [diff] [review]
0001-Bug-964349-Move-CrashReporter.annotateCrashReport-ca.patch, v2

> (In reply to :Gavin Sharp (email [e-mail]) from comment #2)
> > nit: you can use Services.appinfo.annotateCrashReport
> 
> Done.

No, you didn't.

Also, I think we should do this in XULBrowserWindow.onStateChange in browser.js, as it seems completely independent from the tabbrowser internals.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #5)
> > (In reply to :Gavin Sharp (email [e-mail]) from comment #2)
> > > nit: you can use Services.appinfo.annotateCrashReport
> > 
> > Done.
> 
> No, you didn't.

Oops, must have uploaded an older patch version.

> Also, I think we should do this in XULBrowserWindow.onStateChange in
> browser.js, as it seems completely independent from the tabbrowser internals.

Ok, fair enough.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> > Also, I think we should do this in XULBrowserWindow.onStateChange in
> > browser.js, as it seems completely independent from the tabbrowser internals.

This isn't notified when switching tabs though? We should not only update the crash reporter URL when the top frame's URL in the visible tab changes but also when switching tabs. The same thing goes for docShell swapping, that's why I thought it would be better to call it from updateCurrentBrowser() instead of registering all kinds of listeners.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> (In reply to Tim Taubert [:ttaubert] from comment #6)
> > > Also, I think we should do this in XULBrowserWindow.onStateChange in
> > > browser.js, as it seems completely independent from the tabbrowser internals.
> 
> This isn't notified when switching tabs though?

It is in some cases (see the updateCurrentBrowser implementation). There's also XULBrowserWindow.onUpdateCurrentBrowser.

Comment 9

[Dão Gottwald [:dao]]

I'm not even sure why this is tied to onStateChange, though. onLocationChange seems like a natural fit.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-964349-Move-CrashReporter.annotateCrashReport-ca.patch, v3

You're right, XULBrowserWindow.onLocationChange() seems like what we want.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(CCing bsmedberg/kairo just as a heads up in case they encounter any weirdness/changes to URL annotations in crash reports)

Comment 12

[(not currently active) Ted Mielczarek]

I think we work under the impression that URLs in crash reports are best-effort, since it's possible to crash from any tab or frame that's loaded anyway.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8366894 [details] [diff] [review]
0001-Bug-964349-Move-CrashReporter.annotateCrashReport-ca.patch, v3

>@@ -3726,16 +3720,32 @@ var XULBrowserWindow = {
>     gGestureSupport.restoreRotationState();
> 
>     // See bug 358202, when tabs are switched during a drag operation,
>     // timers don't fire on windows (bug 203573)
>     if (aRequest)
>       setTimeout(function () { XULBrowserWindow.asyncUpdateUI(); }, 0);
>     else
>       this.asyncUpdateUI();
>+
>+#ifdef MOZ_CRASHREPORTER
>+    if (aLocationURI) {
>+      let uri = aLocationURI.clone();
>+      try {
>+        // If the current URI contains a username/password, remove it.
>+        uri.userPass = "";
>+      } catch (ex) { /* Ignore failures on about: URIs. */ }

Is it even possible that the URI contains the username and password at this point?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #13)
> >+#ifdef MOZ_CRASHREPORTER
> >+    if (aLocationURI) {
> >+      let uri = aLocationURI.clone();
> >+      try {
> >+        // If the current URI contains a username/password, remove it.
> >+        uri.userPass = "";
> >+      } catch (ex) { /* Ignore failures on about: URIs. */ }
> 
> Is it even possible that the URI contains the username and password at this
> point?

Yes, I just checked by loading ftp://username:password@hostname/. That would be reported as is.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/066920d21e0e

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/04499ec92ec9 alongside backouts for the three patches from bug 936271 for breaking mochitest-browser-chrome like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=33845117&tree=Fx-Team

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/78925fbd7e9c

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Followed up with:

https://hg.mozilla.org/integration/fx-team/rev/15fd49561a6e

to replace Services.appinfo.annotateCrashReporter() with using gCrashReporter because Services.appinfo is missing the right QI.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78925fbd7e9c
https://hg.mozilla.org/mozilla-central/rev/15fd49561a6e