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.
