Closed Bug 741255 Opened 10 years ago Closed 10 years ago

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

Categories

(Toolkit :: Telemetry, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jruderman, Assigned: andreshm)

References

Details

(Keywords: assertion, regression, Whiteboard: [mentor=jdm][lang=js][fuzzblocker])

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 717069
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.
Whiteboard: [mentor=jdm][lang=js]
Whiteboard: [mentor=jdm][lang=js] → [mentor=jdm][lang=js][fuzzblocker]
Attached patch Patch (obsolete) — Splinter Review
Attachment #618763 - Flags: review?
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.
Attachment #618763 - Flags: review?
Attached patch Patch v2 (obsolete) — Splinter Review
Updated patch.
Attachment #618763 - Attachment is obsolete: true
Attachment #621169 - Flags: review?(josh)
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.
Attachment #621169 - Flags: review?(josh)
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #621169 - Attachment is obsolete: true
Attachment #621194 - Flags: review?(josh)
Comment on attachment 621194 [details] [diff] [review]
Patch v3

Looks good; thanks!
Attachment #621194 - Flags: review?(josh) → review?(taras.mozilla)
Attachment #621194 - Flags: review?(taras.mozilla) → review?(nfroyd)
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.
Attachment #621194 - Flags: review?(nfroyd) → review+
Attached patch patch v4Splinter Review
Updated patch.
Attachment #621194 - Attachment is obsolete: true
Attachment #621707 - Flags: review?(nfroyd)
Attachment #621707 - Flags: review?(nfroyd) → review+
(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
Keywords: checkin-needed
Will push this in a sec.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/49cbdb1ea4e2
Assignee: nobody → andres
Status: NEW → ASSIGNED
Whiteboard: [mentor=jdm][lang=js][fuzzblocker] → [mentor=jdm][lang=js][fuzzblocker][fixed-in-fx-team]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/49cbdb1ea4e2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=jdm][lang=js][fuzzblocker][fixed-in-fx-team] → [mentor=jdm][lang=js][fuzzblocker]
You need to log in before you can comment on or make changes to this bug.