Make strings in nsIStackFrame API sane

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla29
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 7 obsolete attachments)

No description provided.
Posted patch stackFrame.patch (obsolete) — Splinter Review
Maybe this breaks something. I'm waiting for a try result.
Attachment #8358789 - Flags: review?(bzbarsky)
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.
Attachment #8358789 - Flags: review?(bzbarsky) → review+
Posted patch stackFrame.patch (obsolete) — Splinter Review
This patch contains also the mMessage part.
Attachment #8358789 - Attachment is obsolete: true
Attachment #8358898 - Flags: review?(bzbarsky)
Posted patch stackFrame.patch (obsolete) — Splinter Review
I want to add a test for this IsVoid vs IsEmpty
Attachment #8358898 - Attachment is obsolete: true
Attachment #8358898 - Flags: review?(bzbarsky)
Attachment #8358926 - Flags: review?(bzbarsky)
Posted patch stackFrame.patch (obsolete) — Splinter Review
Attachment #8358926 - Attachment is obsolete: true
Attachment #8358926 - Flags: review?(bzbarsky)
Attachment #8358956 - Flags: review?(bzbarsky)
Posted patch stackFrame.patch (obsolete) — Splinter Review
green on try.
Attachment #8358956 - Attachment is obsolete: true
Attachment #8358956 - Flags: review?(bzbarsky)
Attachment #8359159 - Flags: review?(bzbarsky)
Any chance of an interdiff from the already-reviewed patch here?
Flags: needinfo?(amarchesini)
Posted patch interdiff (obsolete) — Splinter Review
interdiff
Attachment #8359470 - Flags: review?(bzbarsky)
Flags: needinfo?(amarchesini)
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.
Attachment #8359470 - Flags: review?(bzbarsky) → review+
Attachment #8359159 - Flags: review?(bzbarsky)
Summary: sanitize strings in nsIStackFrame API → Make strings in nsIStackFrame API sane
Posted patch stackFrame.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=baf61b807ee3
Attachment #8359159 - Attachment is obsolete: true
Attachment #8359470 - Attachment is obsolete: true
> >+  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.
Posted patch UUIDs updatedSplinter Review
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.