Looks a lot like a stale TODO. Comments on the bug sound like we went through roughly the same sequence of plans, insights, and backtracking as you just did.
On Tue, Feb 16, 2016 at 8:54 AM, Alex Kodat <[email protected]> wrote: > Well, I feel like an idiot. Tried making the change and promptly blew out > in test-thread-termination/ErrorObjectAfterTermination. The reason is that > the native Error functions can't run if TerminateExecution has been called. > So the "normal" case where the Exception::Error functions won't return a > Local<Object> is when they return an Undefined because of > TerminateExecution. I initially thought that the fact that this exposes the > fact that the Exception::Error functions use native JS is a bit groty but > came to the realization that this is reasonably good behavior. If you want > to Throw a new Exception in C++ code but are TerminateExecution has been > called, you get an Undefined back and if you just Throw it, the termination > continues so life is good. The fact that you've lost the exception you > meant to throw is kind of irrelevant at this point. > > If you intend to Set properties in an Error object returned by the > Exception::Error functions and are vulnerable to TerminateExecution then > you better make sure you got an object back. Any C++ API calls that don't > use native functions work fine, even after TerminateExecution which is a > good thing as otherwise it would be a nightmare writing C++ API code where > TerminateExecution is a possibility. > > One thing I'm a little puzzled about is the comment in > test-thread-termination.cc/ErrorObjectAfterTermination: "// > TODO(yangguo): crbug/403509. Check for empty handle instead" (any relation > ;-)). Is there a plan/thought to have Exception::Error return an empty > handle in the TerminateExecution case (I couldn't locate crbug/403509)? It > seems that the current behavior of returning Undefined might be better. > > Thanks > > On Monday, February 15, 2016 at 3:40:25 AM UTC-8, Yang Guo wrote: >> >> I guess it would be ok to have it return Local<Object> and crash on >> anything else inside the API call. It would not break the API since >> existing users of this should just compile just fine. >> >> Accepting patches :) >> >> On Thursday, February 11, 2016 at 7:58:06 PM UTC+1, Alex Kodat wrote: >>> >>> Would it be a waste of everyone's time if I created an issue and >>> submitted a change to make Error return Local<Object>? FWIW, my inclination >>> would make Factory::NewError return a JSObject and go from there though if >>> people fee that's a bridge too far, I'd just put a checked cast into >>> api.cc. It just seems kinda crazy for Factor::NewError to happily return to >>> its caller if the situation is so grim it can't return an error object. In >>> fact, I kinda wonder about it catching an exception thrown by the native >>> constructor and returning that to the caller. It's really hard to picture >>> what would be going on in such a scenario and if there's anything sensible >>> to do beyond falling on one's sword. >>> >>> Or maybe this is all too trivial to bother with in which case I >>> appreciate the discussion, anyway. >>> >>> On Thursday, February 11, 2016 at 4:10:39 AM UTC-8, Yang Guo wrote: >>>> >>>> The issue is... with other parts of the API, when we return a >>>> MaybeLocal, it may have thrown an exception, in which process we created an >>>> Error object, and return an empty handle as result. >>>> >>>> This case is special. We want to create an Error object. If that itself >>>> fails, throwing an exception makes no sense, since we cannot create another >>>> Error object. We don't expect this to happen unless bootstrapping hasn't >>>> finished yet, or we ran out of stack space, or something to that effect. >>>> >>>> I guess we could turn that into a MaybeLocal<Object>, but I don't >>>> really think we should break the API for this small detail. That's just my >>>> opinion though. >>>> >>>> Yang >>>> >>>> On Wednesday, February 10, 2016 at 6:39:44 PM UTC+1, Alex Kodat wrote: >>>>> >>>>> First, I'll confess I'm not a huge fan of MaybeLocal but, leaving that >>>>> aside, even if I accept the utility of MaybeLocal I would expect it to be >>>>> used for errors for which there's a reasonable hope of recovery and some >>>>> way of actually testing the recovery code. >>>>> >>>>> If Exception::Error returns an empty result, the world (or at least >>>>> the Isolate) has turned to yogurt and I feel I can't rely on any API calls >>>>> actually working so it seems like there's not much for me to do other than >>>>> crashing and I would have preferred the crash closer to the point where >>>>> the >>>>> construction of Error actually failed (Factory::NewError). Beyond that, >>>>> short of an extremely artificial test where I say hack the Error native >>>>> constructors, I can't see how I could possibly test any error recovery >>>>> code >>>>> for an Exception::Error failure. To me this sort of error seems closer to >>>>> out of storage errors than unusual results errors. >>>>> >>>>> All that said, I guess MaybeLocal<Object> Exception::Error is better >>>>> than Local<Value> or MaybeLocal<Value> as it better documents what an >>>>> embedder should expect though the former might be accompanied by a comment >>>>> that the ToLocalChecked on the return value should only ever fail in the >>>>> event of catastrophic errors so you might not want to expend much >>>>> bandwidth >>>>> on worrying about it. >>>>> >>>>> On Wednesday, February 10, 2016 at 7:55:01 AM UTC-8, Daniel Vogelheim >>>>> wrote: >>>>>> >>>>>> Generally, the API tries hard to pass errors up. >>>>>> >>>>>> I wonder if we should return MaybeLocal<Object>, then. There's been a >>>>>> huge APi refactoring in the past to deprecate returning empty Locals (or >>>>>> Undefined, or so) as error markers, and instead signal all such failures >>>>>> by >>>>>> returning an empty MaybeLocal. Not quite what Alex asks for, but IMHO >>>>>> more >>>>>> consistent with the remainder of the API. >>>>>> >>>>>> >>>>>> On Wed, Feb 10, 2016 at 4:26 PM, Alex Kodat < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> Thanks for that. I suspected as much. Is v8 really doing embedder's >>>>>>> a favor by exposing such a catastrophe to them? Presumably, if >>>>>>> Factory::NewError fails, we're out of storage (which v8 correctly >>>>>>> doesn't >>>>>>> do embedders the favor of exposing to them), there's some other >>>>>>> catastrophic failure (like say the embedder's code has run amok >>>>>>> clobbering >>>>>>> the v8 heap), or the native Error constructors have a problem >>>>>>> (inconceivable except maybe if a developer was fiddling with them). >>>>>>> >>>>>>> None of these seem like cases where it's useful to share the pain >>>>>>> with the embedder code, especially as the situation is presumably that >>>>>>> the >>>>>>> embedder code wants to reflect an error to the JS but whoops, all it can >>>>>>> get is an undefined from Exception::Error. Now what? >>>>>>> >>>>>>> Could I/should I open an issue for this? Sorry if this is a stupid >>>>>>> question, I'm still not quite sure when it's appropriate to post to the >>>>>>> list or open an issue. While I know you can code the change about as >>>>>>> fast >>>>>>> as I can hit the Post button, I'd be happy to make the change myself. >>>>>>> FWIW, >>>>>>> I'd do the type-checking and cast in Factory::NewError and have it >>>>>>> (them) >>>>>>> return a JSObject as it seems like it might be useful for other V8 code >>>>>>> to >>>>>>> be able to count on NewError giving it an object (like for example, the >>>>>>> JSON parser?). Maybe this is too trivial to waste anyone's bandwidth on? >>>>>>> >>>>>>> On Wednesday, February 10, 2016 at 5:25:29 AM UTC-8, Yang Guo wrote: >>>>>>>> >>>>>>>> This probably never happens, but in case creating the error object >>>>>>>> fails, undefined is returned. >>>>>>>> >>>>>>>> On Monday, February 8, 2016 at 9:03:42 PM UTC+1, Alex Kodat wrote: >>>>>>>>> >>>>>>>>> This must have been asked before but can't find an explanation so >>>>>>>>> ... just curious why Exception::Error et al are declared to have a >>>>>>>>> Local<Value> result instead of Local<Object>. A not uncommon pattern >>>>>>>>> is to >>>>>>>>> create a new Error object and then set some properties on it which >>>>>>>>> requires >>>>>>>>> a ->ToObject or Local<Object>::Cast on the Exception::Error result. >>>>>>>>> Trivial, but it just seems odd that it's necessary. >>>>>>>>> >>>>>>>>> Thanks >>>>>>>>> >>>>>>>> -- >>>>>>> -- >>>>>>> v8-users mailing list >>>>>>> [email protected] >>>>>>> http://groups.google.com/group/v8-users >>>>>>> --- >>>>>>> You received this message because you are subscribed to the Google >>>>>>> Groups "v8-users" group. >>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>> send an email to [email protected]. >>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>> >>>>>> >>>>>> -- > -- > v8-users mailing list > [email protected] > http://groups.google.com/group/v8-users > --- > You received this message because you are subscribed to the Google Groups > "v8-users" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- -- v8-users mailing list [email protected] http://groups.google.com/group/v8-users --- You received this message because you are subscribed to the Google Groups "v8-users" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
