1014473 - 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

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fixing the warning

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(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 4

[Nathan Froyd [:froydnj]]

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.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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

Comment 6

[Nathan Froyd [:froydnj]]

(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?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9b67e77397f2

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/9b67e77397f2

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rename `state`

As requested, the companion patch that makes stuff a little clearer.

Comment 10

[Nathan Froyd [:froydnj]]

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.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rename `state`, v2

Same one, but with a search and replace /mayGetState/fetchState/g.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a16b26c855d8

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8429573 [details] [diff] [review]
Rename `state`, v2

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

Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0b3884af83f