Closed
Bug 1388917
Opened 7 years ago
Closed 7 years ago
PlaceholderTransaction's constructor fails to move SelectionState properly
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file)
2.00 KB,
patch
|
masayuki
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8895591 -
Flags: review?(masayuki)
Comment 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 5•7 years ago
|
||
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?
Comment 6•7 years ago
|
||
(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...
Assignee | ||
Comment 7•7 years ago
|
||
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. :-(
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/13521db456ac
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 9•7 years ago
|
||
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+
Comment 10•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/4f7144901630
Comment 11•7 years ago
|
||
(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.
Description
•