Script evaluate/callFunction fails for rejected promises
Categories
(Remote Protocol :: WebDriver BiDi, defect, P1)
Tracking
(firefox114 fixed)
Tracking | Status | |
---|---|---|
firefox114 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
Details
(Whiteboard: [webdriver:m7][webdriver:relnote])
Attachments
(3 files)
Our BiDi script evaluation helpers currently fail to handle basic rejected promises such as
var rej = Promise.reject();
rej.then(() => {})
There is no stack attached in this case and we fail to build the exception details at https://searchfox.org/mozilla-central/rev/26790fecfcda622dab234b28859da721b80f3a35/remote/webdriver-bidi/modules/windowglobal/script.sys.mjs#81-113
Comment 1•2 years ago
|
||
Interesting. I assume this would block bug 1828930? What about using catch()
as I stated on the other bug already?
Assignee | ||
Comment 2•2 years ago
|
||
(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #1)
Interesting. I assume this would block bug 1828930? What about using
catch()
as I stated on the other bug already?
Using catch does not trigger the error, since we don't return an exception anymore. However we can't use this in the test, because as discussed the problem is actually that we should not allow the fetch helper to timeout.
So I don't think this should block bug 1828930. Fixing this Bug would probably only change the symptom.
Now I'm not sure yet what we should do here. The BiDi spec explicitly asks for ExceptionDetails if we return an exception.
script.ExceptionDetails = {
columnNumber: js-uint,
exception: script.RemoteValue,
lineNumber: js-uint,
stackTrace: script.StackTrace,
text: text,
};
So I was looking at how we could get the right context here. For rejected promises (eg just Promise.reject("error")
) we read return.promiseResolutionSite
which gives us a stack.
However in this case (Promise.reject("error").then(() => {})
) even if the promiseState
is "rejected"
, promiseResolutionSite
is null.
But we could use promiseAllocationSite
instead. This will point to the context which created the then(() => {})
which is actually the promise we return in this case.
I think this is the right thing to do. Based on the documentation of promiseResolutionSite
, this property will only be set if the promise itself was rejected or resolved. But here since we used then
, we are not returning the promise which was rejected but an equivalent promise.
Will think about this a bit more, I am getting slightly lost in https://tc39.es/ecma262/#await .
Assignee | ||
Comment 3•2 years ago
|
||
Hi James,
When an expression or function returns a rejected promises on which we called then()
, we can easily find the stacktrace for the allocation of the returned promise (ie where then()
was called), but not the site where the original promise was rejected.
For instance for script.callFunction with the following functionDeclaration + awaitPromise set to true
() => {
const rejectedPromise = Promise.reject("failed");
// do something else
return rejectedPromise.then(() => {});
}
We get exceptionDetails with lineNumber: 3, corresponding to the rejectedPromise.then
and not to the Promise.reject
. Whereas with the following functionDeclaration:
() => {
const rejectedPromise = Promise.reject("failed");
// do something else
return rejectedPromise;
}
lineNumber is set 1, corresponding to the Promise.reject call.
Since then() creates a new Promise I think it makes sense to return the allocation site corresponding the this new promise, but I would prefer to check with you before changing the implementation.
Comment 4•2 years ago
|
||
So the spec has no opinion whatsoever on this; the stack is really up to the implementation. It is of course possible that clients end up depending on the details of the stack in particular implementations, but that's perhaps less of a concern for WebDriver compared to ECMAScript since you have to opt-in to supporting each browser (although of course we'd like that opt-in to be as trivial as possible).
In terms of what's most useful, I imagine that seeing where the promise was rejected would be the most useful thing. But I don't know that it matters much without specific user input.
Assignee | ||
Comment 5•2 years ago
|
||
Thanks for the feedback!
Was thinking more about this, and I actually feel like both are important. For instance, when trying to fix unhandled promise rejections in tests, it was more important to find the actual promise which was not handled rather than the original rejection. I didn't want to silence the original failure/exception, but I needed to handle the actual promise which was missing a catch handler.
Having both would be the best, but short of that I think it's fair to use the allocation site, and should be relevant. I'll move forward with this approach for now.
Assignee | ||
Comment 6•2 years ago
|
||
Depends on D176354
Updated•2 years ago
|
Assignee | ||
Comment 7•2 years ago
|
||
Depends on D176353
Assignee | ||
Comment 8•2 years ago
|
||
Updated•2 years ago
|
Comment 11•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/533ca11677bb
https://hg.mozilla.org/mozilla-central/rev/16aa7403ac04
https://hg.mozilla.org/mozilla-central/rev/2f8178038080
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Updated•1 years ago
|
Description
•