WARNING: At least one completion condition is taking too long to complete. Conditions: Error getting state: TypeError: state is not a function at safeGetState@resource://gre/modules/AsyncShutdown.jsm:118:5

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

No description provided.
Summary: ARNING: At least one completion condition is taking too long to complete. Conditions: Error getting state: TypeError: state is not a function at safeGetState@resource://gre/modules/AsyncShutdown.jsm:118:5 → WARNING: At least one completion condition is taking too long to complete. Conditions: Error getting state: TypeError: state is not a function at safeGetState@resource://gre/modules/AsyncShutdown.jsm:118:5
Assignee: nobody → dteller
Attachment #8426907 - Flags: review?(nfroyd)
Comment on attachment 8426907 [details] [diff] [review]
Fixing the warning

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

::: toolkit/modules/AsyncShutdown.jsm
@@ +630,5 @@
>        // we need to notify the user.
>        let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>        timer.initWithCallback(function() {
>          let msg = "At least one completion condition is taking too long to complete." +
> +	  " Conditions: " + JSON.stringify(this.state) +

This doesn't seem correct: state is either a function or null.  We check this in Barrier.addBlocker.  Why are we getting something else here?

It also seems unhelpful when state *is* a function, because stringifying a function...well, the documentation says that it's omitted or nulled in an object or array, respectively, but doesn't say anything about what happens at toplevel.  I am inclined to believe that nothing good happens.

So this fixes the error, I guess, but drops debuggability on the floor.
Attachment #8426907 - Flags: review?(nfroyd) → review-
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 8426907 [details] [diff] [review]
> Fixing the warning
> 
> Review of attachment 8426907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/modules/AsyncShutdown.jsm
> @@ +630,5 @@
> >        // we need to notify the user.
> >        let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> >        timer.initWithCallback(function() {
> >          let msg = "At least one completion condition is taking too long to complete." +
> > +	  " Conditions: " + JSON.stringify(this.state) +
> 
> This doesn't seem correct: state is either a function or null.  We check
> this in Barrier.addBlocker.  Why are we getting something else here?

Because it's not `state`, it's `this.state`, which is a getter that returns all the states of all pending conditions.
Comment on attachment 8426907 [details] [diff] [review]
Fixing the warning

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

Argh, too many things named 'state' and 'addBlocker' and whatnot in this file.  You're right, and we use JSON.stringify in our *other* timer timeout function.  My mistake.
Attachment #8426907 - Flags: review- → review+
Yes, I pondered changing a few names but decided not to do it in this file, to avoid muddying the patch.
Try: https://tbpl.mozilla.org/?tree=Try&rev=3082e2a01a41
Keywords: checkin-needed
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on May 16th-23th) from comment #5)
> Yes, I pondered changing a few names but decided not to do it in this file,
> to avoid muddying the patch.

Could you please file a bug with the result of your ponderings?

Updated

5 years ago
Blocks: 1012924
https://hg.mozilla.org/mozilla-central/rev/9b67e77397f2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla32
Posted patch Rename `state` (obsolete) — Splinter Review
As requested, the companion patch that makes stuff a little clearer.
Attachment #8428742 - Flags: feedback?(nfroyd)
Comment on attachment 8428742 [details] [diff] [review]
Rename `state`

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

WFM, though I like |stateFetcher| or |stateDescriptor| or similar better than |mayGetState|; the latter sounds more like a boolean to me.
Attachment #8428742 - Flags: feedback?(nfroyd) → feedback+
Same one, but with a search and replace /mayGetState/fetchState/g.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a16b26c855d8
Attachment #8428742 - Attachment is obsolete: true
Attachment #8429573 - Flags: review?(nfroyd)
Comment on attachment 8429573 [details] [diff] [review]
Rename `state`, v2

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

Thanks!
Attachment #8429573 - Flags: review?(nfroyd) → review+
You need to log in before you can comment on or make changes to this bug.