1185589 - Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Biru [:poiru]]

Over in bug 1182723, we noticed that there seems to be an unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding. The `commands = ...` lines are actually assigning to mSingleLineCommands because `commands` is a reference to it.

Comment 1

[Biru [:poiru]]

Attachment: Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Comment 2

[Tom S [:evilpie]]

Comment on attachment 8636104 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Sorry, can't review this.

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8636104 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Probably the nicest bug in Firefox that I ever produced.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8636104 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

> -  nsTArray<mozilla::CommandInt>& commands = mSingleLineCommands;

I don't understand why this causes unintentional assignment. Could you explain about that?

> -  nsTArray<mozilla::CommandInt>& commands = mSingleLineCommands;
> +  const nsTArray<mozilla::CommandInt>* commands = nullptr;

...

>      case nsIWidget::NativeKeyBindingsForRichTextEditor:
> -      commands = mRichTextCommands;
> +      commands = &mRichTextCommands;
>        break;
>    }
> +  MOZ_ASSERT(commands);

I think that you actually changes the behavior. In the old code, commands references to mSingleLineCommands if aType is invalid (or defined new one but here is not maintained!) but your new code sets nullptr.

And you checks if commands is a valid pointer only on debug build. So, if actually this becomes buggy, the code will try to execute nullptr->IsEmpty() in release build.

Please add default case and insert MOZ_CRASH() in the default case.

Comment 5

[Biru [:poiru]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> Comment on attachment 8636104 [details] [diff] [review]
> Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding
> 
> > -  nsTArray<mozilla::CommandInt>& commands = mSingleLineCommands;
> 
> I don't understand why this causes unintentional assignment. Could you
> explain about that?

`commands` is a reference to mSingleLineCommands. So, e.g. `commands = mMultiLineCommands` is equivalent to `mSingleLineCommands = mMultiLineCommands`, which is probably not what was intended.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Birunthan Mohanathas [:poiru] from comment #5)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> > Comment on attachment 8636104 [details] [diff] [review]
> > Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding
> > 
> > > -  nsTArray<mozilla::CommandInt>& commands = mSingleLineCommands;
> > 
> > I don't understand why this causes unintentional assignment. Could you
> > explain about that?
> 
> `commands` is a reference to mSingleLineCommands. So, e.g. `commands =
> mMultiLineCommands` is equivalent to `mSingleLineCommands =
> mMultiLineCommands`, which is probably not what was intended.

Oh, really? I misunderstood the C++ reference's behavior.

Comment 7

[Biru [:poiru]]

Attachment: Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8636410 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Thank you!

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Per discussions, this is related to bug 1182723 and should go in with it on the same branches.

Comment 10

[Biru [:poiru]]

Attachment: esr38 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is landing along with bug 1182723, which is a sec-high bug.
Fix Landed on Version: 42
Risk to taking this patch (and alternatives if risky): Minimal
String or UUID changes made by this patch: No

Comment 11

[Biru [:poiru]]

Comment on attachment 8636410 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

See comment 10.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bcf65c04b69c
https://hg.mozilla.org/mozilla-central/rev/b38a3f669d49

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb80ce0eb9c8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4febcc97a5d6

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/f6da6a262fca

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/da1458e68958
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/da1458e68958
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/def978d33afc
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/def978d33afc
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/90e98e683875
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/90e98e683875