lazy async in the migration guide
Hey Folks,
the migration guide says devs can use processNextEvent to spin the event
loop to handle async calls in synchronous functions. This method is
known to cause stability problems or even crashes. I would not like to
recommend this practice, and it is in fact something I would normally
reject for in an AMO review. Are there any objections to removing this
trick from the migration guide?
https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57
Philipp
On 07/06/2018 09:56, Philipp Kewisch wrote:
the migration guide says devs can use processNextEvent to spin the event
loop to handle async calls in synchronous functions. This method is
known to cause stability problems or even crashes. I would not like to
recommend this practice, and it is in fact something I would normally
reject for in an AMO review. Are there any objections to removing this
trick from the migration guide?
Yes, I object. It's is the last resort for some add-on developers, and
we in fact have this code in the product:
https://dxr.mozilla.org/comm-beta/source/mail/base/content/mailWindowOverlay.js#3764.
Sadly this was necessary for Mac, but has since been removed in trunk.
See https://hg.mozilla.org/comm-central/rev/90deaec87201.
https://dxr.mozilla.org/comm-beta/source/mailnews/base/src/nsMessenger.cpp#338
I'm fine to label it with a big disclaimer.
Jörg.
On 07.06.18 10:22, Jörg Knobloch wrote:
On 07/06/2018 09:56, Philipp Kewisch wrote:
the migration guide says devs can use processNextEvent to spin the event
loop to handle async calls in synchronous functions. This method is
known to cause stability problems or even crashes. I would not like to
recommend this practice, and it is in fact something I would normally
reject for in an AMO review. Are there any objections to removing this
trick from the migration guide?
Yes, I object. It's is the last resort for some add-on developers, and
we in fact have this code in the product:
https://dxr.mozilla.org/comm-beta/source/mail/base/content/mailWindowOverlay.js#3764.
Sadly this was necessary for Mac, but has since been removed in trunk.
See https://hg.mozilla.org/comm-central/rev/90deaec87201.
https://dxr.mozilla.org/comm-beta/source/mailnews/base/src/nsMessenger.cpp#338
I'm fine to label it with a big disclaimer.
You may want to use nsIJSInspector instead. It's a debugger API, but
it's generally usable.
Interface:
https://dxr.mozilla.org/mozilla-central/source/devtools/platform/nsIJSInspector.idl
Creation:
Cc["@mozilla.org/jsinspector;1"].createInstance(Ci.nsIJSInspector);
-Patrick
I'm just concerned we are recommending this, when it will then be rejected during AMO review. This is a frustrating developer experience. The fact that we use this code in production doesn't make it less of a stability issue.
How do you propose we resolve this conflict?
Philipp
On 7. Jun 2018, at 11:47 AM, Patrick Brunschwig patrick@enigmail.net wrote:
On 07.06.18 10:22, Jörg Knobloch wrote:
On 07/06/2018 09:56, Philipp Kewisch wrote:
the migration guide says devs can use processNextEvent to spin the event
loop to handle async calls in synchronous functions. This method is
known to cause stability problems or even crashes. I would not like to
recommend this practice, and it is in fact something I would normally
reject for in an AMO review. Are there any objections to removing this
trick from the migration guide?
Yes, I object. It's is the last resort for some add-on developers, and
we in fact have this code in the product:
https://dxr.mozilla.org/comm-beta/source/mail/base/content/mailWindowOverlay.js#3764.
Sadly this was necessary for Mac, but has since been removed in trunk.
See https://hg.mozilla.org/comm-central/rev/90deaec87201.
https://dxr.mozilla.org/comm-beta/source/mailnews/base/src/nsMessenger.cpp#338
I'm fine to label it with a big disclaimer.
You may want to use nsIJSInspector instead. It's a debugger API, but
it's generally usable.
Interface:
https://dxr.mozilla.org/mozilla-central/source/devtools/platform/nsIJSInspector.idl
Creation:
Cc["@mozilla.org/jsinspector;1"].createInstance(Ci.nsIJSInspector);
-Patrick
Maildev mailing list
Maildev@lists.thunderbird.net
http://lists.thunderbird.net/mailman/listinfo/maildev_lists.thunderbird.net
Am 07.06.2018 um 11:47 schrieb Patrick Brunschwig:
On 07.06.18 10:22, Jörg Knobloch wrote:
On 07/06/2018 09:56, Philipp Kewisch wrote:
the migration guide says devs can use processNextEvent to spin the event
loop to handle async calls in synchronous functions. This method is
known to cause stability problems or even crashes. I would not like to
recommend this practice, and it is in fact something I would normally
reject for in an AMO review. Are there any objections to removing this
trick from the migration guide?
Yes, I object. It's is the last resort for some add-on developers, and
we in fact have this code in the product:
https://dxr.mozilla.org/comm-beta/source/mail/base/content/mailWindowOverlay.js#3764.
Sadly this was necessary for Mac, but has since been removed in trunk.
See https://hg.mozilla.org/comm-central/rev/90deaec87201.
https://dxr.mozilla.org/comm-beta/source/mailnews/base/src/nsMessenger.cpp#338
I'm fine to label it with a big disclaimer.
You may want to use nsIJSInspector instead. It's a debugger API, but
it's generally usable.
Interface:
https://dxr.mozilla.org/mozilla-central/source/devtools/platform/nsIJSInspector.idl
Creation:
Cc["@mozilla.org/jsinspector;1"].createInstance(Ci.nsIJSInspector);
-Patrick
@Patrick: Can you provide a working replacement for the lazy
async-to-sync code listed in the migration guide, using JSInspector?
Thanks
John
On 07.06.18 13:49, John Bieling wrote:
Am 07.06.2018 um 11:47 schrieb Patrick Brunschwig:
On 07.06.18 10:22, Jörg Knobloch wrote:
On 07/06/2018 09:56, Philipp Kewisch wrote:
the migration guide says devs can use processNextEvent to spin the
event
loop to handle async calls in synchronous functions. This method is
known to cause stability problems or even crashes. I would not like to
recommend this practice, and it is in fact something I would normally
reject for in an AMO review. Are there any objections to removing this
trick from the migration guide?
Yes, I object. It's is the last resort for some add-on developers, and
we in fact have this code in the product:
https://dxr.mozilla.org/comm-beta/source/mail/base/content/mailWindowOverlay.js#3764.
Sadly this was necessary for Mac, but has since been removed in trunk.
See https://hg.mozilla.org/comm-central/rev/90deaec87201.
https://dxr.mozilla.org/comm-beta/source/mailnews/base/src/nsMessenger.cpp#338
I'm fine to label it with a big disclaimer.
You may want to use nsIJSInspector instead. It's a debugger API, but
it's generally usable.
Interface:
https://dxr.mozilla.org/mozilla-central/source/devtools/platform/nsIJSInspector.idl
Creation:
Cc["@mozilla.org/jsinspector;1"].createInstance(Ci.nsIJSInspector);
-Patrick
@Patrick: Can you provide a working replacement for the lazy
async-to-sync code listed in the migration guide, using JSInspector?
function PickerShow(fp) {
let inspector = Cc["@mozilla.org/jsinspector;1"].
createInstance(Ci.nsIJSInspector);
let rv, result;
fp.open(result => {
rv = result;
inspector.exitNestedEventLoop();
});
inspector.enterNestedEventLoop(0); /* wait for async process to
terminate */
return rv;
}
I'm using this at several places in Enigmail, and so far it passed any
AMO review (not only those by Philipp ;-) )
-Patrick
On 07/06/2018 13:17, Philipp Kewisch wrote:
The fact that we use this code in production doesn't make it less of a stability issue.
Agreed. What exactly is the issue? Should we go and replace the code in
TB 60 with the "inspector" snippet?
Jörg.
@Phillip:
If I use this, will this pass AMO review?
Am 07.06.2018 um 14:17 schrieb Patrick Brunschwig:
function PickerShow(fp) {
let inspector = Cc["@mozilla.org/jsinspector;1"].
createInstance(Ci.nsIJSInspector);
let rv, result;
fp.open(result => {
rv = result;
inspector.exitNestedEventLoop();
});
inspector.enterNestedEventLoop(0); /* wait for async process to
terminate */
return rv;
}
Hi,
why did this important thread go silent? Did I miss the final statement?
I do see an urgent need for action, if the processNextEvent technique
recommended in an official document* will actually not pass review,
because it is indeed causing stability problems.
Can you promote the nsIJSInspector solution to be the official
replacement? Or are there issues as well?
Thanks,
John
Am 07.06.2018 um 14:17 schrieb Patrick Brunschwig:
function PickerShow(fp) {
let inspector = Cc["@mozilla.org/jsinspector;1"].
createInstance(Ci.nsIJSInspector);
let rv, result;
fp.open(result => {
rv = result;
inspector.exitNestedEventLoop();
});
inspector.enterNestedEventLoop(0); /* wait for async process to
terminate */
return rv;
}
I'd be willing to accept it for now, it seems to be used by the Firefox devtools.
Still, this is a stop gap and should be avoided if at all possible. IIUC this is used in devtools to be able to take action while the debugger is paused, if you have used the devtools then you may know this also can cause stability issues.
Spinning the event loop to wait for an async function to complete breaks the intended workflow of JavaScript and isn't something that shouldn't be used in production.
Philipp
On 18. Jun 2018, at 8:56 AM, John Bieling john.bieling@gmx.de wrote:
Hi,
why did this important thread go silent? Did I miss the final statement? I do see an urgent need for action, if the processNextEvent technique recommended in an official document* will actually not pass review, because it is indeed causing stability problems.
Can you promote the nsIJSInspector solution to be the official replacement? Or are there issues as well?
Thanks,
John
Am 07.06.2018 um 14:17 schrieb Patrick Brunschwig:
function PickerShow(fp) {
let inspector = Cc["@mozilla.org/jsinspector;1"].
createInstance(Ci.nsIJSInspector);
let rv, result;
fp.open(result => {
rv = result;
inspector.exitNestedEventLoop();
});
inspector.enterNestedEventLoop(0); /* wait for async process to
terminate */
return rv;
}