1036777 - Remove useAllocator nastiness in XPCConvert::JSData2Native

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

These branches look like this:

Right below here, we have the lines:

        if (useAllocator) {
            ws = nsXPConnect::GetRuntimeInstance()->NewShortLivedString();
            *((const nsString**)d) = ws;
        } else {
            ws = *((nsString**)d);
        }

This threw me for a loop when going over it, because that cast looks really unsafe. It turns out it is, and the issue is just that we never call this with useAllocator=false for any dipper parameters (which include DOMStrings). We should remove these conditionals, and replace them with MOZ_ASSERTs.

I'll mark this as dependent on bug 1034627 to make jandem's life easier.

Comment 1

[[e-mail]]

(In reply to Bobby Holley from comment #0)
>         if (useAllocator) {
>             ws = nsXPConnect::GetRuntimeInstance()->NewShortLivedString();
>             *((const nsString**)d) = ws;
>         } else {
>             ws = *((nsString**)d);
>         }
> 
> This threw me for a loop when going over it, because that cast looks really
> unsafe. It turns out it is, and the issue is just that we never call this
> with useAllocator=false for any dipper parameters (which include
> DOMStrings). We should remove these conditionals, and replace them with
> MOZ_ASSERTs.

OK, I cheated. Originally there were two code paths (with slightly different code, which was one of the issues I was trying to fix) one saying nsString *ws = nsXPConnect::GetRuntimeInstance()->NewShortLivedString(); *((const nsString**)d) = ws; and the other saying nsAString *ws = *((nsAString**)d); when in fact what I should have written was:
nsAString *ws;
if (useAllocator) {
  nsString *w = nsXPConnect::GetRuntimeInstance()->NewShortLivedString();
  *((const nsString**)d) = ws;
  ws = w;
} else {
  ws = *((nsAString**)d);
}
(Note that this compiles because I only ever call nsAString methods on *ws). But I'm sure that both code paths are used; IIRC one is used for calling C++ from JS and the other is for returning strings from C++ to JS.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native

It turns out the useAllocator argument is only used for the dipper types
(nsXPTType::{T_ASTRING, T_DOMSTRING, T_UTF8STRING, T_CSTRING}), while we
only pass true in cases where we don't have a dipper type:

* XPCConvert::JSArray2Native errors on those types;
* GetNamedPropertyAsVariantRaw() passes an interface type;
* nsXPCWrappedJSClass::CallMethod passes !param.IsDipper() for the first
  calls and only reaches the last call for dependent types, which do not
  include any of the dipper types;
* CallMethodHelper::ConvertIndependentParam handles dipper types earlier
* and CallMethodHelper::ConvertDependentParam handles dependent types.

Comment 3

[[e-mail]]

Attachment: useAllocator = true

Comment 4

[[e-mail]]

Attachment: useAllocator = false

Comment 5

[[e-mail]]

(In reply to comment #3)
> useAllocator = true

This happened to be a T_UTF8STRING in this case.

(In reply to comment #4)
> useAllocator = false

This happened to be a T_CSTRING in this case.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8453720 [details] [diff] [review]
Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native

Review of attachment 8453720 [details] [diff] [review]:
-----------------------------------------------------------------

Ahah! Thanks for sorting this out Ms2ger.

The useAllocator stuff must have become dead when I factored out JS->C++ call dipper conversion into CallMethodHelper::HandleDipperParam in bug 683802. Happy day!

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69c0707a144a

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v1

Comment 9

[[e-mail]]

Comment on attachment 8453984 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v1

>+    if (paramInfo.IsStringClass() && !AllocateStringClass(dp, paramInfo))
This undoes bug 966911 part 1 :-(

Comment 10

[[e-mail]]

(What's so nasty about useAllocator anyway?)

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8453984 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v1

Review of attachment 8453984 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I don't think I feel confident in stamping this.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ms2ger from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69c0707a144a

For the record: backed out in

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6553613d42d

because IsDipper() is false for in parameters.

Comment 13

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=700c7a1a8b5f

Comment 14

[Bobby Holley (:bholley)]

(In reply to [e-mail] from comment #9)
> Comment on attachment 8453984 [details] [diff] [review]
> Part 1 - Stop relying on useAllocator for 'in' string classes in
> ConvertIndependentParam. v1
> 
> >+    if (paramInfo.IsStringClass() && !AllocateStringClass(dp, paramInfo))
> This undoes bug 966911 part 1 :-(

Oh, true. But only a small part of it. The NewShortLivedString stuff means that we still don't allocate for the 16-bit cases, so we're only allocating for the 8-bit cases. And there were various other improvements in that patch too.

As I understand it, the optimization loss of Part 1 here is that we'll have an extra allocation for null/void/empty cstring and utf8string parameters. Is there anything else?

It seems like if we care about this case, we should just add an analogous NewShortLivedCString. But I suspect that we don't.

(In reply to [e-mail] from comment #10)
> (What's so nasty about useAllocator anyway?)

It makes this function absurdly complex. Each time I've had to review a patch to it (twice this year), it's taken me upwards of an hour to page everything in.

With the WebIDL Codegen, this stuff isn't hot in the same way that it used to be. At this point, simplicity and hackability is much more of a priority. Unless we've measured an impact from these optimizations (which bug 966911 comment 1 suggests we haven't), we should steer towards making this code comprehensible, which these two patches definitely do.

Comment 15

[[e-mail]]

Comment on attachment 8453984 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v1

So you're going to clean up all the useAllocator = true cases too? (And by that I don't mean Ms2ger's patch, as it doesn't clean up all the cases.)

>-    if (!XPCConvert::JSData2Native(&dp->val, src, type, true, &param_iid, &err)) {
>+    if (!XPCConvert::JSData2Native(&dp->val, src, type, !paramInfo.IsStringClass(), &param_iid, &err)) {
Why this change?

Comment 16

[Bobby Holley (:bholley)]

(In reply to [e-mail] from comment #15)
> Comment on attachment 8453984 [details] [diff] [review]
> Part 1 - Stop relying on useAllocator for 'in' string classes in
> ConvertIndependentParam. v1
> 
> So you're going to clean up all the useAllocator = true cases too? (And by
> that I don't mean Ms2ger's patch, as it doesn't clean up all the cases.)

Which cases doesn't it clean up? The whole param is gone.

> >-    if (!XPCConvert::JSData2Native(&dp->val, src, type, true, &param_iid, &err)) {
> >+    if (!XPCConvert::JSData2Native(&dp->val, src, type, !paramInfo.IsStringClass(), &param_iid, &err)) {
> Why this change?

So that Ms2ger's patch (as part 2) is pure dead-code removal.

Comment 17

[[e-mail]]

(In reply to Bobby Holley from comment #16)
> (In reply to comment #15)
> > So you're going to clean up all the useAllocator = true cases too? (And by
> > that I don't mean Ms2ger's patch, as it doesn't clean up all the cases.)
> 
> Which cases doesn't it clean up? The whole param is gone.

Sure, but there's a bunch of code that's called by those cases.

> > >-    if (!XPCConvert::JSData2Native(&dp->val, src, type, true, &param_iid, &err)) {
> > >+    if (!XPCConvert::JSData2Native(&dp->val, src, type, !paramInfo.IsStringClass(), &param_iid, &err)) {
> > Why this change?
> 
> So that Ms2ger's patch (as part 2) is pure dead-code removal.

So why didn't you just change it to pass false like the other callers?

Comment 18

[[e-mail]]

Comment on attachment 8453720 [details] [diff] [review]
Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native

>+        nsString* ws = *((nsString**)d);
By the way, as per comment #1, this is only really guaranteed to be an nsAString*.

Comment 19

[[e-mail]]

I tried to gather some quick-and-dirty statistics, and they seem to agree with you.

244 null strings returned from JS to C++.
13 null strings passed from JS to C++.
68 empty strings returned from JS to C++.
7 null strings passed from JS to C++.
117 other strings returned from JS to C++.
1271 other strings passed from JS to C++.

So just over 1% of allocations were saved.

The most popular string type was UTF8String with 860 strings (50%!); AString had 376, DOMString had 357, CString had 127. Maybe we do need an 8-bit short lived string.

Comment 20

[Bobby Holley (:bholley)]

(In reply to [e-mail] from comment #17)
> (In reply to Bobby Holley from comment #16)
> > (In reply to comment #15)
> > > So you're going to clean up all the useAllocator = true cases too? (And by
> > > that I don't mean Ms2ger's patch, as it doesn't clean up all the cases.)
> > 
> > Which cases doesn't it clean up? The whole param is gone.
> 
> Sure, but there's a bunch of code that's called by those cases.

I still don't understand. My plan is to land my patch with Ms2ger's patch on top of it (see the try push in comment 13). Which cases code remain with those two patches applied?

> 
> > > >-    if (!XPCConvert::JSData2Native(&dp->val, src, type, true, &param_iid, &err)) {
> > > >+    if (!XPCConvert::JSData2Native(&dp->val, src, type, !paramInfo.IsStringClass(), &param_iid, &err)) {
> > > Why this change?
> > 
> > So that Ms2ger's patch (as part 2) is pure dead-code removal.
> 
> So why didn't you just change it to pass false like the other callers?

Shrug - just so that the patch is more verifiably correct by inspection. They're equivalent IIUC, and it's a moot point with both patches applied.

Comment 21

[Bobby Holley (:bholley)]

(In reply to [e-mail] from comment #18)
> Comment on attachment 8453720 [details] [diff] [review]
> Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native
> 
> >+        nsString* ws = *((nsString**)d);
> By the way, as per comment #1, this is only really guaranteed to be an
> nsAString*.

Good point. I'll attach an additional patch.

(In reply to [e-mail] from comment #19)
> The most popular string type was UTF8String with 860 strings (50%!); AString
> had 376, DOMString had 357, CString had 127. Maybe we do need an 8-bit short
> lived string.

Good find! This probably changed a lot because most of the APIs with DOMStrings are now on WebIDL. I'll file a followup bug.

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix string casts to be safe per-XPIDL. v1

These types map to nsAString and nsACString respectively.

Comment 23

[[e-mail]]

(In reply to Bobby Holley from comment #20)
> Which cases code remain with those two patches applied?

Actually there was less code than I thought, but both in CleanupParam and DeleteShortLivedString there's code to handle the cases you're removing.

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v2

Now with more code removal.

Comment 25

[[e-mail]]

Comment on attachment 8454783 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v2

>+    // String classes are always "in" - those that are marked "out" are converted
>+    // by the XPIDL compiler to "in+dipper".
>+    if (paramInfo.IsStringClass() && !AllocateStringClass(dp, paramInfo))
>+        return false;
>     if (paramInfo.IsDipper())
>-        return HandleDipperParam(dp, paramInfo);
>+        return true; // See the note above IsDipper in xptinfo.h.
(Is it worth nesting the AllocateStringClass and IsDipper checks inside an IsStringClass block?)

Comment 26

[[e-mail]]

Comment on attachment 8454783 [details] [diff] [review]
Part 1 - Stop relying on useAllocator for 'in' string classes in ConvertIndependentParam. v2

>+
>     // Whether this parameter is passed indirectly on the stack. This mainly
>     // applies to out/inout params, but we use it unconditionally for certain
>     // types.
>     bool IsIndirect() const {return IsOut() ||
>                                GetType().TagPart() == nsXPTType::T_JSVAL;}
> 
>     // NOTE: other activities on types are done via methods on nsIInterfaceInfo
(This patch only has 7 lines of context at the bottom when 8 are expected; the patch tool on one of my build environments is unhappy with this.)

Comment 27

[[e-mail]]

Comment on attachment 8453720 [details] [diff] [review]
Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native

(Has this bitrotted? I seem to need it for bug 1037264 but I can't get it to apply.)

Comment 28

[Bobby Holley (:bholley)]

(In reply to [e-mail] from comment #27)
> Comment on attachment 8453720 [details] [diff] [review]
> Part 2 - Remove the useAllocator argument to XPCConvert::JSData2Native
> 
> (Has this bitrotted? I seem to need it for bug 1037264 but I can't get it to
> apply.)

Yes, it was bitrotted by jandem's patch. I unbitrotted it here:

https://tbpl.mozilla.org/?tree=Try&rev=8db71c6d828c

Comment 29

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f6e47d8296d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/18783d1e2a77
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f06500edd1

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f6e47d8296d
https://hg.mozilla.org/mozilla-central/rev/18783d1e2a77
https://hg.mozilla.org/mozilla-central/rev/a0f06500edd1