1519284 - Rooting hazards in PaymentRequest.cpp and PaymentRequestUpdateEvent.cpp -- Optional<JSObject*>

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, defect)

Description

[Steve Fink [:sfink] [:s:]]

This code:

PaymentDetailsUpdate details;
if (!details.Init(aCx, aValue)) { ... }
nsresult rv = mRequest->IsValidDetailsUpdate(details, true );

is a rooting hazard. details contains an Optional<JSObject*> that could be populated by Init(), then invalidated by a GC in Init() or probably other code, then used in IsValidDetailsUpdate().

I am embarrassed to say that I convinced myself that this was a false positive -- or rather, that this is a false positive:

PaymentDetailsUpdate details;
if (!details.Init(aCx, aValue)) {
  mRequest->AbortUpdate(NS_ERROR_TYPE_ERR);
  JS_ClearPendingException(aCx);
  return;
}

because (1) Optional<JSObject*> isn't populated with anything initially, so that zero-arg ctor really shouldn't be considered to start its live range; and (2) if Init() fails and we run this error handling code, then the analysis will think there's a hazard because the destructor uses 'details' only the destructor won't actually look at any pointers. So I spent a couple of days implementing automatic inference for the first and an annotation for the second, then pushed to try and realized I'm an idiot -- what if Init() doesn't fail? 'details' is populated, and used. I suppose all my work at least makes it report the real hazard and not a false positive.

I don't have the try result back yet, but it had better give me a hazard.

This is autogenerated code. I'm not entirely clear on the correct fix. One option would be to give PaymentDetailsUpdate a trace() method and put 'details' in a Rooted<> here. I don't know if that requires doing the full work required for bug 1517829? Unfortunately this appears to be a real hazard, not a false positive.

Hazard:

Function '_ZN7mozilla3dom25PaymentRequestUpdateEvent16ResolvedCallbackEP9JSContextN2JS6HandleINS4_5ValueEEE$void mozilla::dom::PaymentRequestUpdateEvent::ResolvedCallback(JSContext*, JS::Handle<JS::Value>)' has unrooted 'details' of type 'mozilla::dom::PaymentDetailsUpdate' live across GC call '_ZN7mozilla3dom14PaymentRequest11AbortUpdateE8nsresult$void mozilla::dom::PaymentRequest::AbortUpdate(uint32)' at dom/payments/PaymentRequestUpdateEvent.cpp:66
    PaymentRequestUpdateEvent.cpp:64: Call(24,25, details.PaymentDetailsUpdate())
    PaymentRequestUpdateEvent.cpp:65: Assign(25,26, __temp_10 := aValue*)
    PaymentRequestUpdateEvent.cpp:65: Call(26,27, __temp_9 := details.Init(aCx*,__temp_10*,"Value",0))
    PaymentRequestUpdateEvent.cpp:65: Assume(27,28, !__temp_9*, true)
    PaymentRequestUpdateEvent.cpp:66: Call(28,29, __temp_11 := this*.mRequest.operator->())
    PaymentRequestUpdateEvent.cpp:66: Call(29,30, __temp_11*.AbortUpdate(2152923162)) [[GC call]]
    PaymentRequestUpdateEvent.cpp:67: Call(30,31, JS_ClearPendingException(aCx*))
    PaymentRequestUpdateEvent.cpp:68: Call(31,52, details.~PaymentDetailsUpdate())
GC Function: _ZN7mozilla3dom14PaymentRequest11AbortUpdateE8nsresult$void mozilla::dom::PaymentRequest::AbortUpdate(uint32)
    static already_AddRefed<mozilla::dom::PaymentRequestManager> mozilla::dom::PaymentRequestManager::GetSingleton()
    void mozilla::dom::PaymentRequestManager::PaymentRequestManager() [[complete_ctor]]
    void mozilla::dom::PaymentRequestManager::PaymentRequestManager()
    uint32 mozilla::Preferences::RegisterCallbackAndCall(mozilla::TypedPrefChangeFunc<nsTArray<nsTString<char16_t> > >, int8[38]*, const nsTArray<nsTString<char16_t> >*)[N], T*) [with int N = 38; T = nsTArray<nsTString<char16_t> >; typename mozilla::TypedPrefChangeFunc<T>::Type = mozilla::TypedPrefChangeFunc<nsTArray<nsTString<char16_t> > >]
    uint32 mozilla::Preferences::RegisterCallbackAndCall((void)(int8*,void*)*, nsTSubstring<char>*, void*, uint32)
    IndirectCall: aCallback

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is autogenerated code.

No, it's not. It's handwritten, buggy code. Autogenerated code knows about dictionary rooting and roots them. See https://searchfox.org/mozilla-central/search?q=RootedDictionary&case=true&path=__GENERATED__%2Fdom%2Fbindings. And the autogenerated dictionary in this case already has a TraceDictionary method, because the codegen knows it needs tracing.

And I thought our rooting analysis already caught this stuff; why didn't the tree go red when it landed? :(

I'm not entirely clear on the correct fix.

The correct fix is to use

RootedDictonary<PaymentDetailsUpdate> update(aCx);

as documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Dictionary_types fourth paragraph.

Bug 1285377 kinda tracks switching to a Rooted-like setup, but that wouldn't have helped here, because the basic bug is that the author of this code just didn't root things that need rooting.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So I think the good news is that all this code is disabled on non-nightly builds (hardcoded in PaymentRequest::PrefEnabled), and even on nightly it's disabled by default unless you flip the "dom.payments.request.enabled" preference, and even then it's disabled outside en-US, though by the time you get to that point that's not much comfort.

In particular, I don't think we need to backport this to anywhere, if I'm right about the above.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Yes, Web Payments are disabled by default and development is on indefinite hiatus.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1519284. Fix rooting hazards in PaymentRequest. r=baku

Comment 5

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/autoland/rev/db7342a96476113aa1428e345e704d64c02ab893

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/db7342a96476