Closed
Bug 1014473
Opened 7 years ago
Closed 7 years ago
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
Categories
(Toolkit :: Async Tooling, defect)
Toolkit
Async Tooling
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
Attachments
(2 files, 1 obsolete file)
1.13 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
9.46 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•7 years ago
|
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 | ||
Comment 1•7 years ago
|
||
Assignee: nobody → dteller
Attachment #8426907 -
Flags: review?(nfroyd)
![]() |
||
Comment 2•7 years ago
|
||
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-
Assignee | ||
Comment 3•7 years ago
|
||
(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•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
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
![]() |
||
Comment 6•7 years ago
|
||
(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•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9b67e77397f2
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9b67e77397f2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla32
Assignee | ||
Comment 9•7 years ago
|
||
As requested, the companion patch that makes stuff a little clearer.
Attachment #8428742 -
Flags: feedback?(nfroyd)
![]() |
||
Comment 10•7 years ago
|
||
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+
Assignee | ||
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
Comment on attachment 8429573 [details] [diff] [review] Rename `state`, v2 Review of attachment 8429573 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8429573 -
Flags: review?(nfroyd) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f0b3884af83f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 14•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f0b3884af83f
Whiteboard: [fixed-in-fx-team]
You need to log in
before you can comment on or make changes to this bug.
Description
•