Closed Bug 1222475 Opened 4 years ago Closed 4 years ago

use UniquePtr<T[]> instead of nsAutoArrayPtr<T> in dom/

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files, 1 obsolete file)

This doesn't address dom/{bluetooth,media,xul,xbl}, as the first two are easier
to do on their own and the second two require startup cache changes.
use UniquePtr<T[]> instead of nsAutoArrayPtr<T> in dom/
Attachment #8684259 - Flags: review?(amarchesini)
Comment on attachment 8684259 [details] [diff] [review]
# Attachment to Bug 1222475 - use UniquePtr<T[]> instead of nsAutoArrayPtr<T> in dom/

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

::: dom/canvas/WebGLTextureUpload.cpp
@@ +825,5 @@
>                               " a buffer for doing format conversion.");
>              return;
>          }
>          if (!mContext->ConvertImage(width, height, srcStride, dstStride,
> +                          static_cast<const uint8_t*>(data), convertedData.get(),

can you fix the indentation here?

::: dom/html/HTMLFrameSetElement.cpp
@@ +205,5 @@
>   */
>  nsresult
>  HTMLFrameSetElement::ParseRowCol(const nsAString & aValue,
>                                   int32_t& aNumSpecs,
> +                                 UniquePtr<nsFramesetSpec[]>* aSpecs) 

extra space.

@@ +332,1 @@
>    

remove these extra spaces.

::: dom/html/HTMLFrameSetElement.h
@@ +144,5 @@
>    virtual JSObject* WrapNode(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;
>  
>  private:
>    nsresult ParseRowCol(const nsAString& aValue,
>                         int32_t&         aNumSpecs,

fix the indentation here, removing the extra spaces.
Attachment #8684259 - Flags: review?(amarchesini) → review+
Turns out I can't land this, because the hazard analysis complains:


Function '_ZN28txXPCOMExtensionFunctionCall8evaluateEP14txIEvalContextPP13txAExprResult|uint32 txXPCOMExtensionFunctionCall::evaluate(txIEvalContext*, txAExprResult**)' has unrooted 'invokeParams' of type 'txParamArrayHolder' live across GC call '_ZN28txXPCOMExtensionFunctionCall12GetParamTypeERK14nsXPTParamInfoP16nsIInterfaceInfo|uint32 txXPCOMExtensionFunctionCall::GetParamType(nsXPTParamInfo*, nsIInterfaceInfo*)' at dom/xslt/xpath/txXPCOMExtensionFunction.cpp:419
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:373: Assume(43,48, !__temp_13*, false)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:377: Call(48,49, __temp_15 := methodInfo*.GetParam(0))
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:377: Assign(49,50, __temp_14 := __temp_15*)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:377: Assign(50,51, paramInfo := __temp_14)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:378: Call(51,52, __temp_16 := info.operator 237())
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:378: Call(52,53, type := this*.GetParamType(paramInfo*,__temp_16*))
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:379: Assume(53,58, (type* == 19), false)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:384: Assign(58,59, paramStart := 0)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:385: Assume(59,82, (type* == 17), false)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:406: Assign(82,83, context := 0)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:410: Call(83,84, __temp_21 := this*.field:0.requireParams*((inArgs* - paramStart*),(inArgs* - paramStart*),aContext*))
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:410: Assume(84,89, !__temp_21*, false)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:415: Assign(89,90, i := paramStart*)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:415: Loop(90,91, loop#0)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:415: Assume(1,2, (i* < inArgs*), true)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:416: Call(2,3, __temp_22 := this*.field:0.mParams.field:0.field:0.operator[]((i* - paramStart*)))
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:416: Assign(3,4, expr := __temp_22**)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:418: Call(4,5, __temp_24 := methodInfo*.GetParam(i*))
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:418: Assign(5,6, __temp_23 := __temp_24*)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:418: Assign(6,7, paramInfo:1 := __temp_23)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:419: Call(7,8, __temp_25 := info.operator 237())
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:419: Call(8,9, type:1 := this*.GetParamType(paramInfo:1*,__temp_25*)) [[GC call]]
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:420: Assume(9,10, (type:1* == 19), false)
    dom/xslt/xpath/txXPCOMExtensionFunction.cpp:424: Call(10,11, __temp_26 := invokeParams.operator 239())

AFAICT, this is because txParamArrayHolder was changed from using nsAutoArrayPtr to UniquePtr, and the analysis doesn't understand the semantics of UniquePtr.  Steve, is that correct?
Flags: needinfo?(sphink)
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Turns out I can't land this, because the hazard analysis complains:
> 
> AFAICT, this is because txParamArrayHolder was changed from using
> nsAutoArrayPtr to UniquePtr, and the analysis doesn't understand the
> semantics of UniquePtr.  Steve, is that correct?

Yes, sort of, though it's really the other way around. The analysis *does* understand UniquePtr, and does not understand nsAutoArrayPtr, so it now sees the hazard here. Previously, it would have reported the hazard as a "maybe", specifically in the output file listing "unsafe references", but most of those are false positives so we don't look at them very often.

Anyway, it looks to me like the hazard analysis is correct in your new version. You have a UniquePtr<Foo[]>, where Foo can directly contain GC pointer (in this case, in the form of Values stored within an nsXPTCVariant.) And it's going through the array a value at a time, doing things that certainly look to me like they could GC, which means potentially mangling earlier Values. They're not sitting directly on the stack in this case, but they're in memory that's just as corruptible.

I think the right thing to do here is to root this properly. I can think of two main options for that -- one would be to use a TraceableVector, the other is to make txParamArrayHolder itself Traceable. I chose the latter because I didn't want to figure out what differences might be problematic between txParamArrayHolder's and TraceableVectors's APIs.
Flags: needinfo?(sphink)
Attached patch Root txParamArrayHolder (obsolete) — Splinter Review
froydnj: hopefully you're ok with reviewing the use of AutoJSAPI? I'm not 100% sure about when it's ok to use.

terrence: please review the rooting parts. It might be more generally useful to make nsXPTCVariant Traceable, and use a TraceableVector here, but I was going for a spot fix.
Attachment #8689721 - Flags: review?(terrence)
Attachment #8689721 - Flags: review?(nfroyd)
Assignee: nfroyd → sphink
Status: NEW → ASSIGNED
Comment on attachment 8689721 [details] [diff] [review]
Root txParamArrayHolder

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

Wow, I wasn't sure whether "I chose to do X" was a typo of what you were telling me to do, or an actual "hey, I'm going to fix this."  I'll definitely take the latter, thank you!

bz knows more about virtually all the stuff involved here, so I'm going to defer to him.  It looks fine to my untutored eyes, but you know what they say about knowing what you don't know...
Attachment #8689721 - Flags: review?(nfroyd)
Attachment #8689721 - Flags: review?(bzbarsky)
Attachment #8689721 - Flags: review+
Comment on attachment 8689721 [details] [diff] [review]
Root txParamArrayHolder

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

We should rewrite this on GCVector. As a stopgap this will certainly work, though I'm not particularly happy about the manual .get()[0]. Ugly. It would be better to add a .get(idx) to an Operations mixin, but that's probably overkill for a quick fix.

::: dom/xslt/xpath/txXPCOMExtensionFunction.cpp
@@ +322,5 @@
> +    static void trace(txParamArrayHolder* holder, JSTracer* trc) { holder->trace(trc); }
> +    void trace(JSTracer* trc) {
> +        for (uint8_t i = 0; i < mCount; ++i) {
> +            if (mArray[i].type == nsXPTType::T_JSVAL) {
> +                JS_CallUnbarrieredValueTracer(trc, &mArray[i].val.j, "txParam value");

I feel like we should have a JS::TraceRoot for this case. I guess we don't have that yet though.
Attachment #8689721 - Flags: review?(terrence) → review+
Comment on attachment 8689721 [details] [diff] [review]
Root txParamArrayHolder

You don't need AutoJSAPI here at all, right?  Just nsContentUtils::RootingCxForThread(), at least assuming we don't need to enter requests here or anything like that.

r=me with that, though the AutoJSAPI bits here are in fact fine if you just wanted a random AutoJSAPI.
Attachment #8689721 - Flags: review?(bzbarsky) → review+
Updated to use RootingCxForThread(). I'll leave this here for nfroyd to incorporate. When I imported the other patch, it didn't apply cleanly, so this patch actually has stuff from that patch incorporated into it. Sorry about that. You may need to manually apply.
Attachment #8689721 - Attachment is obsolete: true
Comment on attachment 8689793 [details] [diff] [review]
Root txParamArrayHolder,

Carry over the r+
Attachment #8689793 - Flags: review+
(In reply to Steve Fink [:sfink, :s:] from comment #4)
> Anyway, it looks to me like the hazard analysis is correct in your new
> version. You have a UniquePtr<Foo[]>, where Foo can directly contain GC
> pointer (in this case, in the form of Values stored within an
> nsXPTCVariant.)

I don't see how. I mean, the analysis may think so, but given that we bail out if http://hg.mozilla.org/mozilla-central/annotate/74c7941a9e22/dom/xslt/xpath/txXPCOMExtensionFunction.cpp#l263 returns eUNKNOWN (which it would for T_JSVAL) I don't see how we can have an nsXPTCVariant containing a GC pointer here.
Flags: needinfo?(sphink)
(In reply to Peter Van der Beken [:peterv] from comment #11)
> (In reply to Steve Fink [:sfink, :s:] from comment #4)
> > Anyway, it looks to me like the hazard analysis is correct in your new
> > version. You have a UniquePtr<Foo[]>, where Foo can directly contain GC
> > pointer (in this case, in the form of Values stored within an
> > nsXPTCVariant.)
> 
> I don't see how. I mean, the analysis may think so, but given that we bail
> out if
> http://hg.mozilla.org/mozilla-central/annotate/74c7941a9e22/dom/xslt/xpath/
> txXPCOMExtensionFunction.cpp#l263 returns eUNKNOWN (which it would for
> T_JSVAL) I don't see how we can have an nsXPTCVariant containing a GC
> pointer here.

You are, of course, correct. I hadn't noticed that. So the options are

1. Root it anyway, since rooting rarely affects perf noticeably and you won't have to worry about future changes (however unlikely in this particular chunk of code) from introducing a real hazard.

2. Add in static type info somehow and annotate the analysis so that it knows that it's ok. For example, we could make txParamArrayHolder reject T_JSVAL values itself (eg via a MOZ_ASSERT in the destructor) and then tell the analysis that txParamArrayHolder will never contain any GC pointers. (That would be trivial to do if I could ever manage to land the in-source annotations, but it's still easy enough with the current separate annotations.js file.)

3. Figure out some very narrow way of ignoring this particular case, possibly involving teaching the analysis certain very limited data-flow patterns or something. I've done that before, but this case doesn't seem worth it.

Either of #1 or #2 is fine with me. Let me know if you want my help. (I *think* #2 would mean adding txParamArrayHolder to listNonGCPointers() in annotations.js, but I haven't thought too hard about it.)
Flags: needinfo?(sphink)
Blocks: 1229985
Steve, are you changing the hazard analysis, or am I on the hook for doing the rooting suggested in comment 12?  Your comment was directed at Peter, but since Peter's not doing the work...and you're now the assignee for the bug...I'm confused as to what's going on.
Flags: needinfo?(sphink)
Sorry! I really need to fix bzexport to not automatically set assignee if there already is one. I did not mean to take the bug.

Nathan: I was hoping you would incorporate my patch into yours. I think the easiest/safest approach is still to make it traceable and root it. My patch should be pretty much up to date except you'll now need to rename JS_CallUnbarrieredValueTracer to JS::UnsafeTraceRoot.

But let me know if you run into any trouble.
Assignee: sphink → nfroyd
Flags: needinfo?(sphink)
https://hg.mozilla.org/mozilla-central/rev/8eeb8603b17a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.