Maybe this breaks something. I'm waiting for a try result.

Comment on attachment 8358789 [details] [diff] [review] stackFrame.patch "sanitize" isn't what you mean. How about "use sane strings"? > if (mMessage) { Why not just make mMessage an nsCString also? That's a bit harder for mName, since we do the whole NameAndFormatForNSResult() bit when it's null, but we could go ahead and SetIsVoid() to track the case when we'll need to call that, right? Followup bug on those two is fine, if desired, since sorting out what ToString should do will be a bit annoying. >+NS_IMETHODIMP JSStackFrame::GetName(nsACString& aFunction) You lost the JS_EncodeStringToBuffer bit, which seems bad. Do we have no tests for this stuff? :( Also, you don't need the malloc/adopt dance here. You should just be able to set mFunname's capacity and then write to BeginWriting() on it, no? >+ [optional] in ACString filename, Hmm. I guess that preserves the old behavior, but maybe we should in fact change it and have AUTF8String here? >@@ -899,19 +899,23 @@ nsXPConnect::EvalInSandboxObject(const n This is changing behavior: empty string filename used to be honored here and won't be now. That seems ok, but did you do any checking to verify that? >+++ b/xpcom/base/nsIException.idl >+ readonly attribute ACString languageName; AUTF8String, and same for the others here. All the C++ assume the output is UTF-8, certainly. r=me with those issues fixed.

This patch contains also the mMessage part.

I want to add a test for this IsVoid vs IsEmpty

green on try.

Any chance of an interdiff from the already-reviewed patch here?

interdiff

Comment on attachment 8359470 [details] [diff] [review] interdiff >+++ b/dom/base/DOMError.cpp >+ mName = NS_ConvertUTF8toUTF16(name); NS_CopyUTF8toUTF17(name, mName). Same for message. >+++ b/dom/base/DOMException.cpp >+ aName.Assign(sDOMErrorMsgMap[idx].mName); aName.Rebind(sDOMErrorMsgMap[idx].mName, strlen(sDOMErrorMsgMap[idx].mName)); to avoid the allocation? Same for aMessage. >+++ b/dom/bindings/Exceptions.cpp >+ finalException = new Exception(nsCString(aMessage), aRv, EmptyCString(), nsDependentCString? Or can aMessage be null? >+ if (mFilename.IsEmpty()) { >+ aFilename.SetIsVoid(true); This needs a comment explaining why we need this behavior... > NS_IMETHODIMP JSStackFrame::GetName(nsACString& aFunction) This should probably be doing a CopyUTF16toUTF8 from the JSString's UTF-16 chars to mFunname, not the thing JS_EncodeStringToBuffer does. At least if we care about non-ASCII function names, which I think we should. >+ aFunction.SetIsVoid(true); Likewise. >+++ b/js/xpconnect/src/XPCComponents.cpp >+ nsCOMPtr<nsIException> e = new Exception(nsCString(parser.eMsg), nsDependentCString, unless parser.eMsg can be null. >+++ b/js/xpconnect/src/XPCConvert.cpp >+ msgStr.Assign("<error>"); No, this should still be assigning to "msg" as far as I can tell. r=me with those fixed.

> >+ if (mFilename.IsEmpty()) { > >+ aFilename.SetIsVoid(true); I forgot to write a comment explaining why we do this. I'll do that in a follow up.

Backed out for mochitest crashes. https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0a1d41b80f https://tbpl.mozilla.org/php/getParsedLog.php?id=33171354&tree=Mozilla-Inbound

Comment on attachment 8361758 [details] [diff] [review] stackFrame.patch FYI, here are some examples (not intended to be an exhaustive list) that could have benefited from using AssignLiteral: > static const char defaultLocation[] = "<unknown>"; ... >+ location.Assign(defaultLocation); > static const char js[] = "JavaScript"; > static const char cpp[] = "C++"; > > if (IsJSFrame()) { >- *aLanguageName = (char*) nsMemory::Clone(js, sizeof(js)); >+ aLanguageName.AssignASCII(js); > } else { >- *aLanguageName = (char*) nsMemory::Clone(cpp, sizeof(cpp)); >+ aLanguageName.AssignASCII(cpp); >+ funname.AssignASCII("<TOP_LEVEL>");

https://hg.mozilla.org/mozilla-central/rev/bbaefde28e3b https://hg.mozilla.org/mozilla-central/rev/b7fcaabc7f06