857487 - Deleting a table row horked

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Daniel Glazman (:glazou)]

Removing a table row through inline table editing is horked. This is
a regression.

1. launch a recent Nightly
2. open the Midas demo at http://www-archive.mozilla.org/editor/midasdemo/
3. click on the "View HTML Source" checkbox to switch to source view
4. paste the following markup:

   <table border="1" width="100%"><tbody><tr><td><br></td><td><br></td><td><br></td></tr><tr><td><br></td><td><br></td><td><br></td></tr><tr><td><br></td><td><br></td><td><br></td></tr></tbody></table><br>

5. switch back to wysiwyg view
6. place the caret inside a table cell
7. click on the circular button that appeared in the LEFT edge of the cell

Expected result: row is deleted
Actual result:   no action at all

Comment 1

[Daniel Glazman (:glazou)]

After investigation, this is a regression introduced by fix for bug 781409. The guilty code is this one in nsHTMLEditor::GetCellDataAt:

  nsTableCellFrame* cellFrame =
    tableFrame->GetCellFrameAt(aRowIndex, aColIndex);
  if (!cellFrame)
    return NS_ERROR_FAILURE;

This creates the current bug since nsHTMLEditor::DeleteRow can call nsHTMLEditor::GetCellDataAt with an out-of-bound row and col index without expecting a fail; in particular see comment in line 1321 of nsTableEditor.cpp:

  // We don't fail if we don't find a cell, so this must be real bad

Comment 2

[Daniel Glazman (:glazou)]

cc:ing :tbsaunde who handled bug 781409, cause of this regression.

Comment 3

[Daniel Glazman (:glazou)]

Ok, this also affects all table-row deletions in the editor.
Tweaking bug's title accordingly.
Firefox inline editing, Thunderbird and BlueGriffon all affected.

Comment 4

[Daniel Glazman (:glazou)]

Attachment: trivial fix + test

Comment 5

[(no longer active)]

Comment on attachment 732791 [details] [diff] [review]
trivial fix + test

Review of attachment 732791 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 6

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d428a8ddbdce

Daniel, can you please nominate this for Aurora and Beta approval as well?  Thanks!

Comment 7

[Daniel Glazman (:glazou)]

Comment on attachment 732791 [details] [diff] [review]
trivial fix + test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 857487
User impact if declined: table row deletion in inline editors (Wikipedia, CKEditor, ...), Seamonkey, BlueGriffon non-functional.
Testing completed (on m-c, etc.): tested on m-c and in BlueGriffon's trunk
Risk to taking this patch (and alternatives if risky): extremely low, only the deepest code for table row deletion is touched
String or IDL/UUID changes made by this patch: none

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d428a8ddbdce

Comment 9

[bhavana bajaj [:bajaj]]

Comment on attachment 732791 [details] [diff] [review]
trivial fix + test

Low risk fix for a recent regression.Approving for uplift.

Thanks for the additional tests here :)

Comment 10

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/917b21d005bb
https://hg.mozilla.org/releases/mozilla-beta/rev/5a7ce06c7111

Comment 11

[Mitza]

Verified as fixed on Firefox 23 Nightly(Build ID: 20130408030928) 
	
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:23.0) Gecko/20130408 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130408 Firefox/23.0
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130408 Firefox/23.0

Comment 13

[Mitza]

Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130421 Firefox/23.0   
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:23.0) Gecko/20130421 Firefox/23.0			
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0  
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:23.0) Gecko/20100101 Firefox/23.0

Verified as fixed on Firefox 21 (Build ID:20130416200523) and on Firefox latest nightly (Build ID:20130421031002).

Comment 14

[Mitza]

Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0  
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0

Verified as fixed on Firefox 21 (Build ID:20130425162858).

Comment 15

[Maria Grazia Alastra [:mga]]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:23.0) Gecko/20130428 Firefox/23.0
Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20130428 Firefox/23.0

Verified as fixed on Windows 7 x64 and Ubuntu 12.04 x86_64 with latest Nightly 23.0a1 (Build ID: 20130429030926)

Comment 16

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) gecko/20130503 Firefox/23.0

Verified on Mac - latest Nightly - in addition to Maria's verification on Ubuntu and Windows.