461816 - pressing Ctrl-U in password dialog asserts and then crashes

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: log of assertions and crash, with stacks

Pressing Ctrl-U in the password field of the HTTP auth password dialog crashes the browser.

Steps to reproduce:
 * load https://intranet.mozilla.org/ in a clean profile
 * wait for the HTTP auth dialog to pop up
 * hit tab to get to the password field
 * type a few characters
 * press Ctrl-U to clear those characters

The eventual crash has this at the top of the stack:
nsString::CharAt(unsigned int) const
nsString::operator[](unsigned int) const
nsEditor::DeleteSelectionAndPrepareToCreateNode(nsCOMPtr<nsIDOMNode>&, int&)
nsEditor::CreateTxnForDeleteInsertionPoint(nsIDOMRange*, short,
  EditAggregateTxn*, nsIDOMNode**, int*, int*)
nsEditor::CreateTxnForDeleteSelection(short, EditAggregateTxn**, nsIDOMNode**,
  int*, int*)
nsEditor::DeleteSelectionImpl(short)
nsPlaintextEditor::DeleteSelection(short)
nsSelectionMoveCommands::DoCommand(char const*, nsISupports*)

I'm attaching a log of the assertions with stacks and the crash stack.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This regressed between Linux x86_64 nightlies 2008-10-15-01-mozilla-central and 2008-10-16-08-mozilla-central.

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-10-15+00%3A00%3A00&enddate=2008-10-16+09%3A00%3A00

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 157546 seems like the most likely cause, although bug 302775 is a possibility.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Chris and Peter might be able to help.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So far no luck with reproducing.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #0)
>  * load https://intranet.mozilla.org/ in a clean profile
>  * wait for the HTTP auth dialog to pop up
>  * hit tab to get to the password field
>  * type a few characters
>  * press Ctrl-U to clear those characters
Problem with reproducing is perhaps that ctrl-u doesn't do anything here.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, the non-distro-dependent way to set the pref that is required for Ctrl-U to work is probably:
 * start gconf-editor
 * go to desktop -> gnome -> interface
 * change the "gtk_key_theme" pref to "Emacs"

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I guess I should try with gnome.

Comment 8

[Theppitak Karoonboonyanan]

Attachment: Fixing the missed case of password entry

The case of password box was really missed in my patch for bug 157546. Please try this patch.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #8)
> Please try this patch.

Why are you asking me to try it?  Is it because you think it will fix the bug but you are unable to test it?

Comment 10

[Theppitak Karoonboonyanan]

(In reply to comment #9)
> Why are you asking me to try it?  Is it because you think it will fix the bug
> but you are unable to test it?

Well, I've already tested it, and it works for me. Just want to make sure it's all OK and doesn't cause any more problem on other environments/use-cases.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 345043 [details] [diff] [review]
Fixing the missed case of password entry

It fixes ctrl+u and ctrl+w and mostly ctrl+k.
ctrl+k at the end of the line in a password field asserts
and deletes one char to the left with this patch.

Comment 12

[Theppitak Karoonboonyanan]

(In reply to comment #11)
> (From update of attachment 345043 [details] [diff] [review])
> It fixes ctrl+u and ctrl+w and mostly ctrl+k.
> ctrl+k at the end of the line in a password field asserts
> and deletes one char to the left with this patch.

Pardon me for not being an Emacs fan. What's the purpose of ctrl+k?

Comment 13

[Mats Palmgren (inactive)]

Delete from the caret to the end of the line.  ctrl+k at the end of
a single-line control should do nothing, in a multi-line control it
deletes the newline (so that the next line is appended to the current).
http://kb.mozillazine.org/Emacs_Keybindings_-_Firefox

Comment 14

[Theppitak Karoonboonyanan]

Attachment: Updated patch for ctrl-k

OK. There was some leak in the logic of the last patch. Now nsTextEditRules::WillDeleteSelection() will handle the deletion itself, rather than deferring it to nsPlainTextEditor::DeleteSelection(), which never recognizes the action modifications made by nsPlaintextEditor::ExtendSelectionForDelete() itself.

This should clean up deletion cases.

Comment 15

[Theppitak Karoonboonyanan]

Any problem left? If not, I'll request for a review for checking-in soon.

Comment 16

[Theppitak Karoonboonyanan]

Attachment: Updated patch, without dirty flag

I think the dirty flag is not necessary. Wherever nsPlaintextEditor::ExtendSelectionForDelete() is called, nsEditor::DeleteSelectionImpl() should immediately follows. Deferring it to nsPlaintextEditor::DeleteSelection() is risky, as the aAction argument may be changed by ExtendSelectionForDelete() (via nsTextEditRules::WillDoAction()) without its awareness.

Comment 17

[Theppitak Karoonboonyanan]

Comment on attachment 345435 [details] [diff] [review]
Updated patch, without dirty flag

Please review my patch, thanks.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Peter, can you review the patch here?

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 345435 [details] [diff] [review]
Updated patch, without dirty flag

>diff -r 90e79fb3d8d6 editor/libeditor/text/nsTextEditRules.cpp

>+
>+    res = mEditor->DeleteSelectionImpl(aCollapsedAction);
>+    NS_ENSURE_SUCCESS(res, res);
>+
>+    *aHandled = PR_TRUE;
>+    return NS_OK;

I think it would make sense to join the codepaths in the if and the else that have this block. In the else case return early if bCollapsed is false and then move this block outside of the if/else.

Comment 20

[Theppitak Karoonboonyanan]

Attachment: updated patch, code paths joined [Checkin: Comment 22]

(In reply to comment #19)

> I think it would make sense to join the codepaths in the if and the else that
> have this block. In the else case return early if bCollapsed is false and then
> move this block outside of the if/else.

Indeed. So, please consider this patch.

Comment 21

[Theppitak Karoonboonyanan]

Comment on attachment 345435 [details] [diff] [review]
Updated patch, without dirty flag

Obsolete previous patch.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 349759 [details] [diff] [review]
updated patch, code paths joined
[Checkin: Comment 22]

http://hg.mozilla.org/mozilla-central/rev/f7e30552f3a7

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c07d033ee085