741255 - "ASSERTION: Oops! You're asking for a weak reference to an object that doesn't support that." during shutdown, with telemetry on stack

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Jesse Ruderman]

Attachment: stack trace + DumpJSStack

I get this on almost every shutdown.  It slows down fuzzing.

###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that doesn't support that.: 'factoryPtr', file xpcom/build/nsWeakReference.cpp, line 111

I'm kinda suspicious of bug 717069.

Comment 1

[Josh Matthews [:jdm]]

Despite Taras' feelings about "useless state" in bug 717069, we're going to need to bring those back. This would be a nice easy fix for someone just getting started, and it would make Jesse much happier.

Comment 2

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#655

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: Patch

Comment 4

[Josh Matthews [:jdm]]

While it is true that this patch should hide the assertion that's being seen, it's not the correct fix. The observer for sessionstore-windows-restored is the one that can be removed before the other observers, so we need to store a flag on the TelemetryPing object that stores whether the observer exists or not, and avoid calling removeObserver if it does not exist.

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Updated patch.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 621169 [details] [diff] [review]
Patch v2

Review of attachment 621169 [details] [diff] [review]:
-----------------------------------------------------------------

This looks more correct. Two corrections and it should be ready for a quick review from an actual code owner.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +204,5 @@
>    // Regex that matches histograms we carea bout during startup.
>    _startupHistogramRegex: /SQLITE|HTTP|SPDY|CACHE|DNS/,
>    _slowSQLStartup: {},
>    _prevSession: null,
> +  _windowRestoredObserver : null,

Make this false instead.

@@ +782,5 @@
>    classID: Components.ID("{55d6a5fa-130e-4ee6-a158-0133af3b86ba}"),
> +  QueryInterface: XPCOMUtils.generateQI([
> +    Ci.nsIObserver,
> +    Ci.nsISupportsWeakReference
> +  ])

This change isn't necessary, so it can just be reverted.

Comment 7

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 621194 [details] [diff] [review]
Patch v3

Looks good; thanks!

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 621194 [details] [diff] [review]
Patch v3

Review of attachment 621194 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with one minor nit below.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +204,5 @@
>    // Regex that matches histograms we carea bout during startup.
>    _startupHistogramRegex: /SQLITE|HTTP|SPDY|CACHE|DNS/,
>    _slowSQLStartup: {},
>    _prevSession: null,
> +  _windowRestoredObserver : false,

Please rename this to _hasWindowRestoredObserver to make it clearer that it's a boolean flag.

Comment 10

[Andres Hernandez [:andreshm]]

Attachment: patch v4

Updated patch.

Comment 11

[Raymond Lee [:raymondlee]]

Push to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=619b8d478bf6

Comment 12

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #11)
> Push to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=619b8d478bf6

Passed Try

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Will push this in a sec.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/49cbdb1ea4e2

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/49cbdb1ea4e2