Closed Bug 1829630 Opened 2 years ago Closed 2 years ago

Script evaluate/callFunction fails for rejected promises

Categories

(Remote Protocol :: WebDriver BiDi, defect, P1)

defect
Points:
2

Tracking

(firefox114 fixed)

RESOLVED FIXED
114 Branch
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

Interesting. I assume this would block bug 1828930? What about using catch() as I stated on the other bug already?

Blocks: 1742977, 1750540

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

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.

Flags: needinfo?(james)

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.

Flags: needinfo?(james)

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: nobody → jdescottes
Status: NEW → ASSIGNED
Attachment #9330169 - Attachment description: Bug 1829630 - [bidi] Log warnings for missing stacks in script module → Bug 1829630 - [bidi] Throw explicit error for missing stacks in script module
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/533ca11677bb [bidi] Use promiseAllocationSite as fallback to fetch rejected promise stacktrace r=webdriver-reviewers,Sasha https://hg.mozilla.org/integration/autoland/rev/16aa7403ac04 [bidi] Throw explicit error for missing stacks in script module r=webdriver-reviewers,Sasha https://hg.mozilla.org/integration/autoland/rev/2f8178038080 [wdspec] Test script commands with chained rejected promises r=webdriver-reviewers,Sasha
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39736 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch
Upstream PR merged by moz-wptsync-bot
Severity: -- → S3
Priority: -- → P1
Whiteboard: [webdriver:m7]
Points: --- → 2
Whiteboard: [webdriver:m7] → [webdriver:m7][webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: