Bug 197172 – JSC should have public API for unhandled promise rejections

• Component: JavaScriptCore
• Status: REOPENED

[Michael Anthony]

Hi,

Apologies if this is in the wrong place, but I'm using JavaScriptCore in a native Swift app and I'm unable to catch errors created in an async promise stack. I believe Safari has a global window event, similar to Node's process.on('unhandledRejection'), but I have had no luck finding anything describing how to do this when creating a JSCEngine natively.

[Radar WebKit Bug Importer]

<rdar://problem/50112559>

[Keith Miller]

Unfortunately, I don't think there is anyway to do this right now in JSC. It probably wouldn't be too hard to add a delegate to JSContext that would be called on unhandledRejection though. I can't promise when such an API will happen, however, but it seems like a good idea to have something like that, IMO.

[Yusuke Suzuki]

Created attachment 369669 [details]
Patch

Simple one, just works

[Don Olmstead]

This obsolete now that https://bugs.webkit.org/show_bug.cgi?id=198709 landed?

[Ross Kirsling]

Is this blocked by anything? Turns out we have a need for this too so I'd be happy to assist in getting it done.

[Ross Kirsling]

Created attachment 373561 [details]
Finish Yusuke's PoC

[Ross Kirsling]

Created attachment 373564 [details]
Patch based on microtask queue

[Ross Kirsling]

This patch completes the other half of Yusuke's proof of concept.

This is my first time dealing with the public API, so here's hoping I've not made too many mistakes with regard to best practices. :)

In particular, Yusuke mentioned that we need to GC-mark the held promises but I don't understand concretely what is missing.

[Keith Miller]

Comment on attachment 373564 [details]
Patch based on microtask queue

View in context: https://bugs.webkit.org/attachment.cgi?id=373564&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        This patch allows callbacks for unhandled and late-handled promise rejections to be set via JSC's public C API.
> +        Since there is no event loop in such an environment, these callbacks fire off of the microtask queue.
> +        Callbacks receive the promise and rejection reason as arguments and their return value is ignored.

To be clear, we do assume there's a run loop in the C API. We just don't have a way to configure it beyond the default of using the one associated with the thread the VM was created on. We use that run loop for GC work and for resolving Wasm compilations. 

I think we should use the same queue for this... but I could be convinced otherwise.

[Ross Kirsling]

Created attachment 373962 [details]
WIP patch based on run loop

[Ross Kirsling]

Created attachment 374097 [details]
WIP patch based on run loop

[Ross Kirsling]

Created attachment 374102 [details]
Minimal PromiseDeferredTimer usage example

So if vm.promiseDeferredTimer is the correct way to schedule something on the run loop, then I think there must be something else that I'm not understanding, because I don't see how this can result in a usable API.

Here's a patch that's as minimal as I could make it. Every time you call `Promise.reject()` from the JSC REPL, it schedules some text to be printed. In order to have the printing actually occur, however, we have to make sure that the REPL calls runRunLoop after each iteration.

It's possible that this would be acceptable for the REPL case, but the actual goal here is to provide a public API for registering unhandled rejection callbacks. It seems rather ghastly if this new API is to require another API that the embedder must call every single time they want these callbacks to actually fire. But even if that were how it has to be, I'm not actually sure how to implement said other API without running into the must-NOT-be-holding-a-lock ASSERT:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp#L87

[Keith Miller]

(In reply to Ross Kirsling from comment #12)
> Created attachment 374102 [details]
> Minimal PromiseDeferredTimer usage example
> 
> So if vm.promiseDeferredTimer is the correct way to schedule something on
> the run loop, then I think there must be something else that I'm not
> understanding, because I don't see how this can result in a usable API.
> 
> Here's a patch that's as minimal as I could make it. Every time you call
> `Promise.reject()` from the JSC REPL, it schedules some text to be printed.
> In order to have the printing actually occur, however, we have to make sure
> that the REPL calls runRunLoop after each iteration.

Unless I'm missing something, you don't have to call a second API. That API is just for testing purposes so that we break out of the runloop when we are done testing. Any application with a runloop will be running JSC out of a runloop anyway so they shouldn't need to call that API. We don't do so from the CLI because it's a CLI tool rather than a full app. I think we should design our APIs around use in full applications rather than run-once tools as that's probably the vast majority of use cases.

> 
> It's possible that this would be acceptable for the REPL case, but the
> actual goal here is to provide a public API for registering unhandled
> rejection callbacks. It seems rather ghastly if this new API is to require
> another API that the embedder must call every single time they want these
> callbacks to actually fire. But even if that were how it has to be, I'm not
> actually sure how to implement said other API without running into the
> must-NOT-be-holding-a-lock ASSERT:
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/
> PromiseDeferredTimer.cpp#L87

Since this is a diagnostic API I don't think it's the end of the world. That said, I could see expecting this to run before my application exits, which the runloop based version makes more complicated. One option could be to check for ready promises when releasing the VM lock. We'll need to verify this behavior is valid WRT WebCore though. e.g. if wasm compilations are ok to run after micro tasks have been drained but before the rest of WebCore is consulted. There might also be other weirdness; I have to think about it more.

That assertion is there because we are going to run a runloop, which can run for an unbounded amount of time. If we are holding the API lock, it's possible that the VM will be locked indefinitely and the application could deadlock. It's also not a fundamental assertion just a this is possibly a bug if you hit it one.

[Ross Kirsling]

(In reply to Keith Miller from comment #13)
> Unless I'm missing something, you don't have to call a second API. That API
> is just for testing purposes so that we break out of the runloop when we are
> done testing. Any application with a runloop will be running JSC out of a
> runloop anyway so they shouldn't need to call that API. We don't do so from
> the CLI because it's a CLI tool rather than a full app. I think we should
> design our APIs around use in full applications rather than run-once tools
> as that's probably the vast majority of use cases.

That makes sense; perhaps the focal point of my confusion then is just that I don't know how to make my API test valid. 😅

[Ross Kirsling]

(In reply to Ross Kirsling from comment #12)
> But even if that were how it has to be, I'm not
> actually sure how to implement said other API without running into the
> must-NOT-be-holding-a-lock ASSERT
Hmm, apparently I was wrong about this part -- introducing a JSGlobalContextRunRunLoop call does "just work" for TestAPI.

(Actually, for Cocoa, it suffices to call RunLoop::current().runForDuration(0_s), but confusingly RunLoop::iterate() doesn't work on other platforms -- for instance, the Windows message queue just never sees any messages, regardless of whether I try blocking on the current thread or make a new one...)



(In reply to Ross Kirsling from comment #14)
> That makes sense; perhaps the focal point of my confusion then is just that
> I don't know how to make my API test valid. 😅
I was also wrong about this -- given the above, it's easy to make the test valid; making it representative of application usage is another story.

Concretely, we have a sample app for our platform that does two things:
1. create two threads, each of which executes a script and serves as an inspection target
2. start remote inspector server and (using a hacked-in downstream API) kick off the RunLoopGeneric that it requires

Using the WIP patch allows us to set an unhandled rejection callback on either of the inspection targets, but that callback will never fire.
If we add in JSGlobalContextRunRunLoop and call it on every iteration of that thread's while(true) loop, then things do seem to work.

Perhaps I was just confused by how brainlessly simple the microtask-queue-based approach is to use, and this *would* in fact be acceptable for application usage if we were to give it the name JSGlobalContextHandlePendingTasks instead?


But if not, then I surely don't know how to determine what the correct answer is; I actually still don't think I even understand what "Any application with a runloop will be running JSC out of a runloop anyway so they shouldn't need to call that API." means. (I initially thought it meant that this would all magically work in an application context based on something else that they're expected to do. Maybe that's true for ObjC or something? But it's clearly not true elsewhere.)

[Ross Kirsling]

Hmm, it appears that another way to make the sample app I mentioned work is to call that "hacked-in downstream API" -- i.e. just RunLoop::run() -- on the individual JSContext threads.

So then it would seem like what we require is a public API to call one of the following:
1. vm.promiseDeferredTimer->runRunLoop()
2. RunLoop::run()
3. RunLoop::iterate()

The first is probably too specific and the second is probably unrealistic if it implies relinquishing all control. So then perhaps it's just a matter of finding a way to implement RunLoop::iterate() for all platforms and exposing it to the user?

[Ross Kirsling]

(Note that my previous comments weren't rhetorical, but... :P)

Allow me to go back to square one here and ask something I would've asked much earlier had I started this effort from scratch:

If we look at what WebCore and node-jsc do, the existing issue could simply be phrased as "since globalObjectMethodTable is a private matter, there is no public API by which an embedder can provide their own promiseRejectionTracker implementation".

In this thread, we've been assuming that promiseRejectionTracker is something with a singular expected implementation and therefore copy-pasting it from WebCore down into JSC would be appropriate so long as we change the scheduling mechanism. But then if it's turning out that the scheduling mechanism isn't fully contained within JSC anyway (in the sense that we need to design a whole additional API in order for the embedder to be able to use it), then should we just have them register a whole promiseRejectionTracker instead, instead of registering JSFunctions for individual event callbacks?

After all, my design here was predicated upon Yusuke's first patch, in which the embedder needs not worry about timing, thanks to the microtask queue. If we're now thinking that they *should* worry about timing, via their own lowercase-r run loop which they need for other things anyway, that's fine, but doesn't that mean the capital-R WTF::RunLoop was just a red herring this whole time?

One way or another, I need to know what target I'm aiming for, if there's any hope of hitting it. :D

[Ross Kirsling]

After all that turmoil, it seems that my confusion all stemmed from something very simple:

WTF::RunLoop never checks USE(CF) because Windows platforms are USE(WINDOWS_EVENT_LOOP) && USE(CF), while JSC::RunLoopTimer / JSC::PromiseDeferredTimer exclusively check USE(CF).

While I think this is something that needs to be reworked in a future patch, it turns out that defining a JSRunLoopIterate() with a USE(CF)-conditional implementation suffices to make this patch work on Windows.

[Ross Kirsling]

Created attachment 375197 [details]
Patch

[Darin Adler]

Comment on attachment 375197 [details]
Patch

This isn’t great for iOS and macOS where we have an Objective-C and Swift API that we are not adding to here.

[Ross Kirsling]

(In reply to Darin Adler from comment #20)
> Comment on attachment 375197 [details]
> Patch
> 
> This isn’t great for iOS and macOS where we have an Objective-C and Swift
> API that we are not adding to here.

I was hoping that JSGlobalContextSetUnhandledRejectionCallback would be useful regardless of what sort of application it is -- is that not the case? (Or more broadly, does "anything that can be platform-agnostic ought to be" not apply along this particular boundary?)

Either way, Keith had also voiced concern to me over IRC regarding JSRunLoopIterate not being useful for CF platforms and suggested perhaps exposing a flag that would cause `vm.promiseDeferredTimer->runRunLoop()` to be called after microtask drain instead.

(He also suggested roping in Geoff for further opinions.)

To be frank, I find it sort of disheartening that we have WTF::RunLoop and yet JSC ignores it completely when it can, accessing CFRunLoop directly and expecting embedders to do the same, creating what seems like an unnecessarily-fundamental divide between embedder platforms. :(

Maybe the original microtask queue-based solution wasn't so bad? ;)

[Geoffrey Garen]

> To be frank, I find it sort of disheartening that we have WTF::RunLoop and
> yet JSC ignores it completely when it can, accessing CFRunLoop directly and
> expecting embedders to do the same, creating what seems like an
> unnecessarily-fundamental divide between embedder platforms. :(

There's no reason it has to be this way!

I believe some of this behavior developed organically because the original CF use in JSC predated WTF::RunLoop. Now that we have WTF::RunLoop, let's use it!

[Darin Adler]

(In reply to Ross Kirsling from comment #21)
> I was hoping that JSGlobalContextSetUnhandledRejectionCallback would be
> useful regardless of what sort of application it is -- is that not the case?
> (Or more broadly, does "anything that can be platform-agnostic ought to be"
> not apply along this particular boundary?)

These C functions are not part of the API on iOS and macOS. They are considered internal implementation details on those platforms. So, on those platforms we often need to add Objective-C API that serves the same purpose. That’s what I was referring to.

[Ross Kirsling]

(In reply to Geoffrey Garen from comment #22)
> I believe some of this behavior developed organically because the original
> CF use in JSC predated WTF::RunLoop. Now that we have WTF::RunLoop, let's
> use it!

This is relieving to hear -- I didn't realize the timeline. :)
I suppose I'll handle that in a separate patch though, if the solution here doesn't depend on it.


(In reply to Darin Adler from comment #23)
> These C functions are not part of the API on iOS and macOS. They are
> considered internal implementation details on those platforms. So, on those
> platforms we often need to add Objective-C API that serves the same purpose.
> That’s what I was referring to.

Wow, I had no idea, but I see what you're saying -- I can add wrappers to JSContext.mm if that suffices.

Actually, if that's the case, then would JSRunLoopIterate still be reasonable to add *just* for C and not for ObjC, if the concern is purely that Cocoa platforms won't use it? (Or is this bad form, if perhaps the C API is meant to be a strict subset?)

[Ross Kirsling]

(In reply to Ross Kirsling from comment #24)
> Actually, if that's the case, then would JSRunLoopIterate still be
> reasonable to add *just* for C and not for ObjC, if the concern is purely
> that Cocoa platforms won't use it? (Or is this bad form, if perhaps the C
> API is meant to be a strict subset?)

Digging further, it seems like if nothing else, maybe it could be moved to JSBasePrivate alongside JSReportExtraMemoryCost?

[Ross Kirsling]

Created attachment 375965 [details]
Patch (run loop)

[Ross Kirsling]

Comment on attachment 375965 [details]
Patch (run loop)

Moved JSRunLoopIterate to JSBasePrivate, added ObjC wrappers for callback registration.

[Ross Kirsling]

Ping?

[Ross Kirsling]

Created attachment 376286 [details]
Patch (microtask queue)

[Ross Kirsling]

Comment on attachment 376286 [details]
Patch (microtask queue)

Keith informed me that we might be okay with the original microtask queue-based approach after all (which would remove any cross-platform worries for the time being) -- if so, we can go with this patch instead.

[Ross Kirsling]

Created attachment 376833 [details]
Patch

[Ross Kirsling]

Comment on attachment 376833 [details]
Patch

Keith suggested dropping the "rejectionhandled" API if nobody's requesting it, so here's the simplest patch yet -- it only tracks unhandled rejections and fires off the microtask queue.

[Darin Adler]

Comment on attachment 376833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376833&action=review

This looks good to me, but I think a JavaScript engine expert should probably do the formal review+ duties.

> Source/JavaScriptCore/API/JSContext.h:184
> +@discussion Similar to window.addEventListener('unhandledrejection'), but for a standalone JSContext.

I think it’s confusing that this method documentation does not mention that, for the JSContext from a web page, this callback will never be called because WebKit will handle it. The documentation acknowledges the existence of web browsing by mentioning the event listener on the window object, but then doesn’t clarify that this method is only relevant when there is no window object, essentially "when JavaScript is used outside a webpage context".

I’m not sure the right way to word this, but I think it needs to be mentioned.

> Source/JavaScriptCore/API/JSContext.mm:269
> +    if (exception)
> +        [self notifyException:exception];

Is it OK to not have a "return" here? Why?

> Source/JavaScriptCore/API/JSContextRef.h:162
> +@discussion JSC-only alternative to window.addEventListener('unhandledrejection').

Here we get slightly closer by saying "JSC-only". But the same comment as above applies.

[Keith Miller]

(In reply to Darin Adler from comment #33)
> Comment on attachment 376833 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376833&action=review
> 
> This looks good to me, but I think a JavaScript engine expert should
> probably do the formal review+ duties.
> 
> > Source/JavaScriptCore/API/JSContext.h:184
> > +@discussion Similar to window.addEventListener('unhandledrejection'), but for a standalone JSContext.
> 
> I think it’s confusing that this method documentation does not mention that,
> for the JSContext from a web page, this callback will never be called
> because WebKit will handle it. The documentation acknowledges the existence
> of web browsing by mentioning the event listener on the window object, but
> then doesn’t clarify that this method is only relevant when there is no
> window object, essentially "when JavaScript is used outside a webpage
> context".
> 
> I’m not sure the right way to word this, but I think it needs to be
> mentioned.

I actually don't think this is an issue because the only APIs that provide a JSContext and a window are officially deprecated. WKWebView does not provide any JS objects to the native application.

[Keith Miller]

Comment on attachment 376833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376833&action=review

> Source/JavaScriptCore/API/JSContext.h:187
> +- (void)setUnhandledRejectionCallback:(JSValue *)function;

I'm not sure we should have this function in the ObjC API. I think it might be nicer if the API was: when creating a JSContext, if the JSContextRef does not already have an unhandledRejectionCallback set a function that calls the exceptionHandler callback. There is some downside that we can't provide the promise object. Although, I suppose we could provide that as an additional "currentArguments" value on the JSContext.

>> Source/JavaScriptCore/API/JSContext.mm:269
>> +        [self notifyException:exception];
> 
> Is it OK to not have a "return" here? Why?

Yeah, there needs to be a return here.

> Source/JavaScriptCore/API/JSContextRef.h:167
> +JS_EXPORT void JSGlobalContextSetUnhandledRejectionCallback(JSGlobalContextRef ctx, JSObjectRef function, JSValueRef* exception);

This needs an JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA)) annotation.

[Keith Miller]

Comment on attachment 376833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376833&action=review

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:772
> +    if (promise->inherits<JSInternalPromise>(vm))

We normally do this via jsDynamicCast<JSInternalPromise>(vm, promise). We have optimizations for some cases there that we haven't applied to inherits and it makes it easier to search for.

> Source/JavaScriptCore/runtime/VM.h:1072
> +    Vector<Strong<JSPromise>> m_aboutToBeNotifiedRejectedPromises;

I feel like this is going to be a huge list for non-trivial async programs. if you have a loop with:

let p = new Promise((resolve, reject) => {
    ...
    reject(p)
});

p will be kept alive until the the microtasks are drained. Which means programs will OOM if they never drain all their microtasks...

I think, at least, you'll need to clear handled promises at the GC flip. See where we call finalizeUnconditionalFinalizers.

[Ross Kirsling]

(In reply to Keith Miller from comment #36)
> > Source/JavaScriptCore/runtime/VM.h:1072
> > +    Vector<Strong<JSPromise>> m_aboutToBeNotifiedRejectedPromises;
> 
> I feel like this is going to be a huge list for non-trivial async programs.
>
> if you have a loop with:
>
> let p = new Promise((resolve, reject) => {
>     ...
>     reject(p)
> });
> 
> p will be kept alive until the the microtasks are drained. Which means
> programs will OOM if they never drain all their microtasks...

Is this concern to unique the non-WebCore case...?

WebCore uses DOMGuarded<JSPromise> instead of Strong<JSPromise>, but otherwise this is meant to be the same -- in particular, WebCore processes m_aboutToBeNotifiedRejectedPromises right after microtask checkpoint (even though it subsequently schedules the *reporting* on the event loop).

https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/RejectedPromiseTracker.cpp#L134-L145

[Ross Kirsling]

(In reply to Ross Kirsling from comment #37)
> Is this concern to unique the non-WebCore case...?

* unique to

(BZ really needs an edit feature... 😭 I spend ages writing a comment and then introduce a typo at the last moment...)

[Ross Kirsling]

(In reply to Keith Miller from comment #35)
> > Source/JavaScriptCore/API/JSContext.h:187
> > +- (void)setUnhandledRejectionCallback:(JSValue *)function;
> 
> I'm not sure we should have this function in the ObjC API. I think it might
> be nicer if the API was: when creating a JSContext, if the JSContextRef does
> not already have an unhandledRejectionCallback set a function that calls the
> exceptionHandler callback. There is some downside that we can't provide the
> promise object. Although, I suppose we could provide that as an additional
> "currentArguments" value on the JSContext.

Don't we want this to be opt-in though? Seems like it would be a huge shift to auto-convert unhandled rejections  to exceptions by default, no?

[Keith Miller]

(In reply to Ross Kirsling from comment #39)
> (In reply to Keith Miller from comment #35)
> > > Source/JavaScriptCore/API/JSContext.h:187
> > > +- (void)setUnhandledRejectionCallback:(JSValue *)function;
> > 
> > I'm not sure we should have this function in the ObjC API. I think it might
> > be nicer if the API was: when creating a JSContext, if the JSContextRef does
> > not already have an unhandledRejectionCallback set a function that calls the
> > exceptionHandler callback. There is some downside that we can't provide the
> > promise object. Although, I suppose we could provide that as an additional
> > "currentArguments" value on the JSContext.
> 
> Don't we want this to be opt-in though? Seems like it would be a huge shift
> to auto-convert unhandled rejections  to exceptions by default, no?

I can see the argument for and against this version. Assuming, unhandledPromiseRejection callbacks are rare this wouldn't really be a breaking change. If they happen all the time, then it seems pretty reasonable to do so. If we find we have a compatibility issue we (on Darwin anyway) could also guard the call with a linked on or after check.

[Keith Miller]

(In reply to Ross Kirsling from comment #37)
> (In reply to Keith Miller from comment #36)
> > > Source/JavaScriptCore/runtime/VM.h:1072
> > > +    Vector<Strong<JSPromise>> m_aboutToBeNotifiedRejectedPromises;
> > 
> > I feel like this is going to be a huge list for non-trivial async programs.
> >
> > if you have a loop with:
> >
> > let p = new Promise((resolve, reject) => {
> >     ...
> >     reject(p)
> > });
> > 
> > p will be kept alive until the the microtasks are drained. Which means
> > programs will OOM if they never drain all their microtasks...
> 
> Is this concern to unique the non-WebCore case...?
> 
> WebCore uses DOMGuarded<JSPromise> instead of Strong<JSPromise>, but
> otherwise this is meant to be the same -- in particular, WebCore processes
> m_aboutToBeNotifiedRejectedPromises right after microtask checkpoint (even
> though it subsequently schedules the *reporting* on the event loop).
> 
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/
> RejectedPromiseTracker.cpp#L134-L145

Hmm, then I won't worry about it for now but can you file a bug on it?

[Ross Kirsling]

Created attachment 376931 [details]
Patch

[Ross Kirsling]

Comment on attachment 376931 [details]
Patch

Addressed feedback modulo outstanding questions.

[Keith Miller]

(In reply to Ross Kirsling from comment #43)
> Comment on attachment 376931 [details]
> Patch
> 
> Addressed feedback modulo outstanding questions.

Sorry, I'm a bit lost. What are the outstanding questions?

[Ross Kirsling]

(In reply to Keith Miller from comment #44)
> Sorry, I'm a bit lost. What are the outstanding questions?

Sorry, I hadn't noticed your latest comments when I wrote that.

(In reply to Keith Miller from comment #41)
> Hmm, then I won't worry about it for now but can you file a bug on it?

Opened bug 201005.

(In reply to Keith Miller from comment #40)
> I can see the argument for and against this version. Assuming,
> unhandledPromiseRejection callbacks are rare this wouldn't really be a
> breaking change. If they happen all the time, then it seems pretty
> reasonable to do so. If we find we have a compatibility issue we (on Darwin
> anyway) could also guard the call with a linked on or after check.

Based on our IRC conversation, I guess we could move the C API to JSContextRefPrivate and postpone the ObjC API. It's a bit unfortunate for the OP of this bug though...

[Ross Kirsling]

Created attachment 376949 [details]
Patch

[Don Olmstead]

(In reply to Ross Kirsling from comment #45)
> (In reply to Keith Miller from comment #40)
> > I can see the argument for and against this version. Assuming,
> > unhandledPromiseRejection callbacks are rare this wouldn't really be a
> > breaking change. If they happen all the time, then it seems pretty
> > reasonable to do so. If we find we have a compatibility issue we (on Darwin
> > anyway) could also guard the call with a linked on or after check.
> 
> Based on our IRC conversation, I guess we could move the C API to
> JSContextRefPrivate and postpone the ObjC API. It's a bit unfortunate for
> the OP of this bug though...

Should we maybe fork this bug and keep this open for Apple folks to actually move it over to public and provide the Obj C interface then?

[Keith Miller]

Comment on attachment 376949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376949&action=review

r=me.

> Source/JavaScriptCore/API/tests/testapi.cpp:521
>  }

Can you add tests for:

1) promise that's rejected then handled synchronously.
2) promise that's rejected then handled in a microtask turn.
3) promise that's rejected in a microtask then handled in a later microtask.
4) promise that's rejected in a microtask then not handled.

> Source/JavaScriptCore/runtime/VM.h:1072
> +    Vector<Strong<JSPromise>> m_aboutToBeNotifiedRejectedPromises;

Can you add a FIXME with a link to the bug you filed?

[Keith Miller]

(In reply to Don Olmstead from comment #47)
> (In reply to Ross Kirsling from comment #45)
> > (In reply to Keith Miller from comment #40)
> > > I can see the argument for and against this version. Assuming,
> > > unhandledPromiseRejection callbacks are rare this wouldn't really be a
> > > breaking change. If they happen all the time, then it seems pretty
> > > reasonable to do so. If we find we have a compatibility issue we (on Darwin
> > > anyway) could also guard the call with a linked on or after check.
> > 
> > Based on our IRC conversation, I guess we could move the C API to
> > JSContextRefPrivate and postpone the ObjC API. It's a bit unfortunate for
> > the OP of this bug though...
> 
> Should we maybe fork this bug and keep this open for Apple folks to actually
> move it over to public and provide the Obj C interface then?

I guess it might be nice for people that are already tracking this bug. We probably shouldn't close this bug at least until we have nailed down an API everyone is happy with.

[Ross Kirsling]

Created attachment 377136 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 377136 [details]
Patch for landing

Clearing flags on attachment: 377136

Committed r249058: <https://trac.webkit.org/changeset/249058>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ross Kirsling]

Reopening until we have an ObjC API.

[Oliver Hunt]

Wait, is this actually a public API? It looks like it's SPI?

[Saam Barati]

(In reply to Oliver Hunt from comment #54)
> Wait, is this actually a public API? It looks like it's SPI?

it's SPI

[Ross Kirsling]

(In reply to Oliver Hunt from comment #54)
> Wait, is this actually a public API? It looks like it's SPI?

Sorry about the confusion -- the title of this bug (which is still open) is accurate but the title of the patch that has been landed is not... :(

(I wonder if it would be worth having the wording in the blog post updated?)

[Michael Anthony]

This has been sitting for quite awhile with no resolution to the ObjC API. Should I open a new bug because this thread now has a patch (which doesn't solve the original issue)?

[Ross Kirsling]

(In reply to Michael Anthony from comment #57)
> This has been sitting for quite awhile with no resolution to the ObjC API.
> Should I open a new bug because this thread now has a patch (which doesn't
> solve the original issue)?

We decided to leave this open until the original issue is in fact fully resolved (comment 49), though a new patch could certainly be submitted in a dependent bug.

[Cheng Runfeng]

So 5 years has gone, where is the public api?