Closed Bug 964349 Opened 6 years ago Closed 6 years ago

Move CrashReporter.annotateCrashReport() calls out of sessionstore

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 936271
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8366052 - Flags: review?(dao)
nit: you can use Services.appinfo.annotateCrashReport
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?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
Attachment #8366052 - Attachment is obsolete: true
Attachment #8366052 - Flags: review?(dao)
Attachment #8366137 - Flags: review?(dao)
Comment on attachment 8366137 [details] [diff] [review]
0001-Bug-964349-Move-CrashReporter.annotateCrashReport-ca.patch, v2

> (In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
Attachment #8366137 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #5)
> > (In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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.
(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.
(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.
I'm not even sure why this is tied to onStateChange, though. onLocationChange seems like a natural fit.
You're right, XULBrowserWindow.onLocationChange() seems like what we want.
Attachment #8366137 - Attachment is obsolete: true
Attachment #8366894 - Flags: review?(dao)
(CCing bsmedberg/kairo just as a heads up in case they encounter any weirdness/changes to URL annotations in crash reports)
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 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?
(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.
Attachment #8366894 - Flags: review?(dao)
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
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.
https://hg.mozilla.org/mozilla-central/rev/78925fbd7e9c
https://hg.mozilla.org/mozilla-central/rev/15fd49561a6e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.