Closed Bug 1547218 Opened 6 months ago Closed 5 months ago

IPDLParamTraits<nsTArray<T>> doesn't work if T = RefPtr<U>

Categories

(Core :: IPC, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: farre, Assigned: Nika)

Details

Attachments

(2 files)

BrowsingContext is refcounted, and using BrowsingContext[] would trigger us using IPDLParamTraits<nsTArray<RefPtr<BrowsingContext>>::WriteInternal which moves the elements of the array into WriteIPDLParam which in turn tries to invoke BrowsingContext::~delete and that is deleted due to it being refcounted.

That is, if we have:

template <typename T>
struct Strip {
  typedef T Type;
};

template <typename T>
struct Strip<RefPtr<T>> {
  typedef T Type;
};

template <typename T>
static void Baz(T* a) {

}

template <typename T>
struct Foo {
  template <typename U>
  static void Bar(U&& a) {
    Baz<typename Strip<T>::Type>(std::forward<U>(a));
  }
};

static void Qux() {
  RefPtr<BrowsingContext> c;
  Foo<RefPtr<BrowsingContext>>::Bar(std::move(c));
}

stuff breaks.

Flags: needinfo?(nika)
Type: defect → task

Seems like a defect to me if valid/reasonable IPDL results in invalid C++.

Type: task → defect

I think this is caused by some unfortunate fallout from the way we perform IPDLParamTraits selection.

I have a local patch which flips around how we handle refcounted type selection here, which should make the behaviour for nsTArray<T> and other compound types perform better.

Assignee: nobody → nika
Flags: needinfo?(nika)

The priority flag is not set for this bug.
:jld, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jld)

This has r+'ed patches; is there anything else that needs to happen to land this?

Flags: needinfo?(jld) → needinfo?(nika)

This bug is currently on the stack after my changes in bug 1540731, which until very recently were blocked on :froydnj's review. My current intention is to not land any of these changes until after the merge, to minimize the chance of fallout.

Flags: needinfo?(nika)

The priority flag is not set for this bug.
:jld, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jld)
Flags: needinfo?(jld)
Priority: -- → P2
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15b4b7f9d97a
Part 1: Handle container IPDLParamTraits types more consistently, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a50b68e0deb2
Part 2: Stop special casing pointer types in ParamTraits specialization, r=froydnj
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1553742
No longer regressions: 1553742
You need to log in before you can comment on or make changes to this bug.