Dictionary binding code fails on assignment.

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jib, Assigned: bz)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment)

track.applyConstraints in Bug 1213517 is (will be) a glorified setter for a (root-free) dictionary, so I have new code that does this:

    mConstraints = aConstraints;

But the binding code from [1] chokes:

void
MediaTrackConstraintSet::operator=(const MediaTrackConstraintSet& aOther)
{
  if (aOther.mBrowserWindow.WasPassed()) {
    mBrowserWindow.Construct();
    mBrowserWindow.Value() = aOther.mBrowserWindow.Value();
  } else {
    mBrowserWindow.Reset();
  }
  mDeviceId = aOther.mDeviceId;
  mFacingMode = aOther.mFacingMode;
  mFrameRate = aOther.mFrameRate;
  mHeight = aOther.mHeight;
  mMediaSource = aOther.mMediaSource;
  if (aOther.mScrollWithPage.WasPassed()) {
    mScrollWithPage.Construct();
    mScrollWithPage.Value() = aOther.mScrollWithPage.Value();
  } else {
    mScrollWithPage.Reset();
  }
  mViewportHeight = aOther.mViewportHeight;
  mViewportOffsetX = aOther.mViewportOffsetX;
  mViewportOffsetY = aOther.mViewportOffsetY;
  mViewportWidth = aOther.mViewportWidth;
  mWidth = aOther.mWidth;
}

Specifically, browserWindow and scrollWithPage cause this assert in Maybe.h:

  template<typename... Args>
  void emplace(Args&&... aArgs)
  {
=>  MOZ_ASSERT(!mIsSome);
    ::new (mStorage.addr()) T(Forward<Args>(aArgs)...);
    mIsSome = true;
  }

It balks at any previous value. Instead, would it work to do:

  mBrowserWindow.Reset();
  if (aOther.mBrowserWindow.WasPassed()) {
    mBrowserWindow.Construct(aOther.mBrowserWindow.Value());
  }

?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MediaStreamTrack.webidl?rev=b53b26799619#44
Yeah, this is just buggy.  Clearly we just never tried actually using the assignment operator outside of a copy constructor for a dictionary with optional members.
mBrowserWindow.Construct(aOther.mBrowserWindow.Value()) didn't use to work because of the imperfect forwarding (const gets involved) that Optional does.  But now that we can do variadic templates and Forward<>, we can fix that part.

But yes, even without seems like we need to unconditionally Reset() and then Construct() as needed.
Created attachment 8689865 [details] [diff] [review]
Fix Optional::Construct to do perfect forwarding, and fix the dictionary assignment operator code for members that can have missing values to not try constructing and already constructed member
Attachment #8689865 - Flags: review?(jib)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8689865 [details] [diff] [review]
Fix Optional::Construct to do perfect forwarding, and fix the dictionary assignment operator code for members that can have missing values to not try constructing and already constructed member

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

Wow, that was fast. Works, thanks!

::: dom/bindings/BindingDeclarations.h
@@ +148,5 @@
>  
>    template <class T1>
>    InternalType& Construct(const T1 &t1)
>    {
>      mImpl.emplace(t1);

With the variadic args, you can remove the other two constructors here, yes? I tried it, and it compiles.
Attachment #8689865 - Flags: review?(jib) → review+
> With the variadic args, you can remove the other two constructors here, yes?

Yes!  I meant to, and forgot.

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a353499d50d

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a353499d50d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.