When we report a rejected respondWith() promise we currently just use nsAutoJSString::Init() to do the conversion from JSVAlue. This will give us [Object object] for a lot of common errors. We should unwrap DOMException and Error at least.

Try to unwrap DOMException and Error, otherwise just convert primitives to strings.

Note, P2 has string changes. Unfortunately I missed the somewhat-common error where a script passes a non-Response, like null or undefined, to .respondWith().

Comment on attachment 8681479 [details] [diff] [review] P1 Unwrap common JS values for reject respondWith() promises. r=bz So this isn't so much "unwrapping" the values (which makes it sounds like we're talking security wrappers or reflectors) but trying to extract file/line/column/message from them in a way that has no side-effects, right? Not sure that good naming is for that. >+ aSourceSpecOut.Assign(nsDependentCString(err->filename)); Can you not just Assign(err->filename)? >+ aMessageOut.Assign(NS_LITERAL_STRING("Error: ")); It might be a TypeError or whatnot. You can look at report->exnType to get that information, right? It's hard to believe we don't already have something like this code hanging around, but making it reusable here might be more pain than just doing this for now. >+ else if(NS_SUCCEEDED(UNWRAP_OBJECT(DOMException, obj, domException)) && >+ domException) { UNWRAP_OBJECT succeeds if and only if it sets the third arg to non-null. So no need for the null-check. >+ domException->GetMessageMoz(aMessageOut); Is there a reason to not grab the filename/linenumber/column from the DOMException? >+ if (jsString.init(aCx, aValue)) { And if not, you want to JS_ClearPendingException(aCx), right? r=me with those nits.

Comment on attachment 8681480 [details] [diff] [review] P2 Report non-respond values passed to FetchEvent.respondWith(). r=bz r=me

(In reply to Boris Zbarsky [:bz] from comment #5) > >+ if (jsString.init(aCx, aValue)) { > > And if not, you want to JS_ClearPendingException(aCx), right? How does the context get a pending exception here? I am extracting the string from a Promise provided value, not an exception off the stack.

(In reply to Boris Zbarsky [:bz] from comment #5) > >+ aMessageOut.Assign(NS_LITERAL_STRING("Error: ")); > > It might be a TypeError or whatnot. You can look at report->exnType to get > that information, right? > > It's hard to believe we don't already have something like this code hanging > around, but making it reusable here might be more pain than just doing this > for now. I switched to using xpc::ErrorReport to do the message formatting for me. > >+ domException->GetMessageMoz(aMessageOut); > > Is there a reason to not grab the filename/linenumber/column from the > DOMException? Added code to extract them if the filename is set on the exception. Also, I changed the message to be "<name>: <message>". For example, "NotFoundError: node not found". I think this makes the error messages a bit easier to understand.

https://hg.mozilla.org/integration/mozilla-inbound/rev/461594bb8ab7 https://hg.mozilla.org/integration/mozilla-inbound/rev/255266023178

Landed on inbound so I have some hope of getting the new string into 44 aurora. If the JS_ClearPendingException() thing turns out to be a real issue I will fix in a follow-up.

Comment on attachment 8681616 [details] [diff] [review] P1 Extract common JS values for rejected respondWith() promises. r=bz Uplift request for P1 and P2. Approval Request Comment [Feature/regressing bug #]: Service workers [User impact if declined]: Needed to provide reasonable error reporting for web developers using service workers. Necessary if we decide to ship in 44 and would be nice to have in our promoted dev edition. [Describe test coverage new/current, TreeHerder]: Existing mochitests pass and do not show any regressions. I tested actual reporting output locally. [Risks and why]: Minimal. Only effects service workers. [String/UUID change made/needed]: One new string for reporting a very common service worker interception problem. This should land in m-c before the original string freeze for 44.

> How does the context get a pending exception here? If nsAutoJSString::init returned false, that means we hit OOM somewhere under it: either linearizing an existing JSString, or allocating the XPCOM string to copy into (under AssignJSString) or allocating a JSString to represent the non-string value we passed in. In all those cases an OOM exception will be set on the JSContext. In other words, the pending exception is because nsAutoJSString::init threw it. This is the general pattern of JSAPI and like methods that take a JSContext and return bool success vs failure: false means there is now an exception on the JSContext. In this case you want to just do the equivalent of catching that exception, because there's not much else you can do with it. > I switched to using xpc::ErrorReport to do the message formatting for me. Ah, excellent. I like that much more. ;) > Added code to extract them if the filename is set on the exception. Looks good, though in practice filename will in fact be empty if !NS_SUCCEEDED, so you could just do the GetFilename without checking its retval, then check !filename.IsEmpty().

Hmm, NI myself to file a follow up on exception issue. Note, though, there are many other places in the tree which do not clear the exception either. So this may be a more widespread problem.

Boris, are all these callers of nsAutoJSString::init() wrong too, then? https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsAutoJSString%3A%3Ainit%28struct+JSContext+*%2C+const+JS%3A%3AValue+%26%29%22 I don't see any of them clearing the pending JS exception.

https://hg.mozilla.org/mozilla-central/rev/461594bb8ab7 https://hg.mozilla.org/mozilla-central/rev/255266023178

> Boris, are all these callers of nsAutoJSString::init() wrong too, then? Looking at those callsites: 1) nsContentPermissionRequestProxy::Allow is in fact busted. It's busted in other ways too: e.g. if !JS_GetProperty it's also leaving a dangling exception on the JSContext. 2) nsAutoJSString::init (the value-only version) is imo also busted. The context is used for more than just rooting, obviously. 3) nsAutoJSString::init, the jsid version, is not a problem per se: it just propagates things up to the caller. Looking at its callers, GetPropertyValuesPairs is correct: it just propagates the exception up to the caller. nsGeolocationSettings::HandleGeolocationPerOriginSettingsChange is broken because it presses on with the loop instead of breaking out of it, and similarly broken for its JS_GetPropertyById and JS_GetProperty calls. If it broke out of the loop immediately, it would be ok, since it then reports the exception from the JSContext before returning. 4) nsGeolocationSettings::HandleGeolocationAlwaysPreciseChange is broken for the same reasons. 5) Key::EncodeJSValInternal is ok as far as it goes: it returns error immediately on this false return. It's called from itself and from Key::EncodeJSVal. The latter is called from SetFromJSVal and AppendItem. The callers of AppendItem are _almost_ OK except the KeyPath::ExtractOrCreateKey one that sometimes turns the retval into NS_OK: that one should clear the exception on aCx (though it's possible we never reach this code in that situation where value is undefined). Also, I didn't trace every single IDB codepath here all the way to its entrypoint; there are lots of them. So it's possible there's more bogosity hiding up-stack. 6) nsJSThunk::EvaluateScript is fine: it returns immediately on failure, and has taken ownership of error reporting on the AutoEntryScript, so reports the exception at that return point. 7) RespondWithHandler::RejectedCallback (before the changes in this bug, looks like) is broken. 8) JavaScriptShared::toVariant returns false on init failure. I'm not sure what it's callers do with it; there are a ton of them, but mostly they propagate the failure on to ... somewhere. WrapperAnswer::fail is a bit suspicious in that it ignores the return value, but a quick look at its callers suggests they all TakeOwnerShipOfErrorReporting on the AutoJSAPI objects they pass it, and fail() is always used like |return fail(...)| so these callers are ok. 9) JSKeyedHistogram_Add is ok: on false return it tries to throw on the JSContext anyway, so if there is an exception there already either it will stay or get replaced but the end state is the same: a JSContext with an exception on it. It then returns false to caller to signal the exception. 10) KeyedHistogram_SnapshotImpl same thing as JSKeyedHistogram_Add.

> nsContentPermissionRequestProxy::Allow is in fact busted. Filed bug 1220679. > nsAutoJSString::init (the value-only version) is imo also busted. Bug 1220682. > nsGeolocationSettings::HandleGeolocationPerOriginSettingsChange is broken > nsGeolocationSettings::HandleGeolocationAlwaysPreciseChange is broken Bug 1220688.

Filed bug 1220728 to fix review issues from comment 14.

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/461594bb8ab7 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/255266023178

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment on attachment 8681616 [details] [diff] [review] P1 Extract common JS values for rejected respondWith() promises. r=bz This landed on Nightly a week ago, let's uplift to Aurora44.

Comment on attachment 8681617 [details] [diff] [review] P2 Report non-response values passed to FetchEvent.respondWith(). r=bz This one too.

https://hg.mozilla.org/releases/mozilla-aurora/rev/2894bcec3447 https://hg.mozilla.org/releases/mozilla-aurora/rev/77efb8bcd129

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2894bcec3447 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/77efb8bcd129