Closed Bug 1388917 Opened 7 years ago Closed 7 years ago

PlaceholderTransaction's constructor fails to move SelectionState properly

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file)

I realized while debugging something unrelated that there is a small regression from bug 1385538 with regards to move semantics.  Since PlaceholderTransaction::mStartSel isn't a Maybe any more, the Move() call doesn't cause a move to actually happen here.
Blocks: 1385538
Comment on attachment 8895591 [details] [diff] [review]
Fix moving of SelectionState in PlaceholderTransaction's constructor

I may not understand what happened with the old code.

I guess that the SelectionState was moved but Maybe<>'s state wasn't moved as expected since * of aSelState, right?
Attachment #8895591 - Flags: review?(masayuki) → review+
Yes, exactly.  The old code used to invoke this move constructor: <https://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/mfbt/Maybe.h#127>  But the new code just calls Move() <https://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/mfbt/Move.h#201> but with a SelectionState& argument, which means the return value's type would be SelectionState&&, so the initializer list statement would now invoke SelectionState's implicitly declared move constructor <http://en.cppreference.com/w/cpp/language/move_constructor>.  Therefore, the state of the Maybe<> remains untouched.

The MOZ_ASSERT() I added in this patch is a "test" of sorts for this, without the rest of the patch the assertion will trigger, with it, it won't.
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13521db456ac
Fix moving of SelectionState in PlaceholderTransaction's constructor; r=masayuki
Comment on attachment 8895591 [details] [diff] [review]
Fix moving of SelectionState in PlaceholderTransaction's constructor

Approval Request Comment
[Feature/Bug causing the regression]: bug 1385538
[User impact if declined]: I'm almost certain that this breaks some behavior in some edge case in the editor code, but it's unclear what exactly.  I found the bug by code inspection.
[Is this code covered by automated tests?]: Yes (the MOZ_ASSERT in the patch.)
[Has the fix been verified in Nightly?]: It just landed.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not at all.
[Why is the change risky/not risky?]:
[String changes made/needed]: None.
Attachment #8895591 - Flags: approval-mozilla-beta?
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away Aug 7-8) from comment #3)
> Yes, exactly.  The old code used to invoke this move constructor:
> <https://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/mfbt/Maybe.h#127>  But the new code
> just calls Move()
> <https://searchfox.org/mozilla-central/rev/
> c329d562fb6c6218bdb79290faaf015467ef89e2/mfbt/Move.h#201> but with a
> SelectionState& argument, which means the return value's type would be
> SelectionState&&, so the initializer list statement would now invoke
> SelectionState's implicitly declared move constructor
> <http://en.cppreference.com/w/cpp/language/move_constructor>.  Therefore,
> the state of the Maybe<> remains untouched.
> 
> The MOZ_ASSERT() I added in this patch is a "test" of sorts for this,
> without the rest of the patch the assertion will trigger, with it, it won't.

Thank you very much for your explanation. However, I worry about similar regression since this is hard to find during review...
Tell me about it.  :-(  C++11 move semantics is extremely difficult to use correctly.  I don't really have any good ideas about how we would have prevented this from happening unfortunately except by treating each call to Move() as super suspicious code...  :-/

In this case this was a basic programming mistake I made, and I have nobody else to blame but myself.  It's sad that our test coverage in editor/ is so poor that this went in unnoticed.  :-(
https://hg.mozilla.org/mozilla-central/rev/13521db456ac
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8895591 [details] [diff] [review]
Fix moving of SelectionState in PlaceholderTransaction's constructor

It seems low risk. Beta56+. Should be in 56 beta 3.
Attachment #8895591 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #5)
> [Is this code covered by automated tests?]: Yes (the MOZ_ASSERT in the
> patch.)
> [Has the fix been verified in Nightly?]: It just landed.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Ehsan's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.