Closed
Bug 461816
Opened 16 years ago
Closed 16 years ago
pressing Ctrl-U in password dialog asserts and then crashes
Categories
(Core :: DOM: Editor, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: dbaron, Assigned: thep)
References
Details
(4 keywords)
Attachments
(2 files, 3 obsolete files)
(deleted),
text/plain; charset=UTF-8
|
Details | |
(deleted),
patch
|
peterv
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
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.
Flags: blocking1.9.1?
Reporter | ||
Comment 1•16 years ago
|
||
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
Reporter | ||
Comment 2•16 years ago
|
||
Bug 157546 seems like the most likely cause, although bug 302775 is a possibility.
Blocks: 157546
Chris and Peter might be able to help.
Comment 4•16 years ago
|
||
So far no luck with reproducing.
Comment 5•16 years ago
|
||
(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.
Reporter | ||
Comment 6•16 years ago
|
||
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•16 years ago
|
||
I guess I should try with gnome.
Assignee | ||
Comment 8•16 years ago
|
||
The case of password box was really missed in my patch for bug 157546. Please try this patch.
Reporter | ||
Comment 9•16 years ago
|
||
(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?
Assignee | ||
Comment 10•16 years ago
|
||
(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•16 years ago
|
||
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.
Assignee | ||
Comment 12•16 years ago
|
||
(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•16 years ago
|
||
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
Assignee | ||
Comment 14•16 years ago
|
||
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.
Attachment #345043 -
Attachment is obsolete: true
Assignee | ||
Comment 15•16 years ago
|
||
Any problem left? If not, I'll request for a review for checking-in soon.
Assignee | ||
Comment 16•16 years ago
|
||
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.
Attachment #345239 -
Attachment is obsolete: true
Assignee | ||
Comment 17•16 years ago
|
||
Comment on attachment 345435 [details] [diff] [review]
Updated patch, without dirty flag
Please review my patch, thanks.
Attachment #345435 -
Flags: review?(peterv)
Peter, can you review the patch here?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Reporter | ||
Updated•16 years ago
|
Whiteboard: [needs review (peterv)]
Assignee: nobody → thep
Updated•16 years ago
|
Attachment #345435 -
Flags: superreview+
Attachment #345435 -
Flags: review?(peterv)
Attachment #345435 -
Flags: review+
Comment 19•16 years ago
|
||
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.
Assignee | ||
Comment 20•16 years ago
|
||
(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.
Attachment #349759 -
Flags: review?(peterv)
Updated•16 years ago
|
Attachment #349759 -
Flags: superreview+
Attachment #349759 -
Flags: review?(peterv)
Attachment #349759 -
Flags: review+
Assignee | ||
Comment 21•16 years ago
|
||
Comment on attachment 345435 [details] [diff] [review]
Updated patch, without dirty flag
Obsolete previous patch.
Attachment #345435 -
Attachment is obsolete: true
Whiteboard: [needs review (peterv)] → [needs landing]
Keywords: checkin-needed
Updated•16 years ago
|
Attachment #349759 -
Attachment description: updated patch, code paths joined → updated patch, code paths joined
[Checkin: Comment 22]
Comment 22•16 years ago
|
||
Comment on attachment 349759 [details] [diff] [review]
updated patch, code paths joined
[Checkin: Comment 22]
http://hg.mozilla.org/mozilla-central/rev/f7e30552f3a7
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
Updated•16 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•16 years ago
|
Flags: in-testsuite?
Comment 23•16 years ago
|
||
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]
Updated•16 years ago
|
Keywords: assertion,
regression
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Updated•16 years ago
|
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
You need to log in
before you can comment on or make changes to this bug.
Description
•