217717 - crash [@ nsHTMLEditor::GetCSSBackgroundColorState] deleting table in compose window

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Vincent Deconinck]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030827
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030827

If you delete a html table from a compose window, for example copy/pasted from a
web page, Mozilla crashes immediately.

Reproducible: Always

Steps to Reproduce:
1. Open a page with a small table in the browser, for example
http://groups.google.com/groups?as_umsgid=3F454ECF.6000403%40mozilla.org
2. Copy the text message to the clipboard, starting for example at "From:
Gervase Markham..." and ending at "...meeting at Burning Man? :-)"
3. Paste this in a new Compose Window
4. Click in the table at the top right which reads "View: Complete Thread (2
articles) - Original Format"
5. Choose "Format / Table / Delete / Table" from the menu


Actual Results:  
Crash

Expected Results:  
The table should have been deleted, but no crash

Note that this causes dataloss, as you potentially have written a long mail and
just wanted to add the cut and paste article at the end ("I'll just remove this
annoying table and send it"). That was my case the first time it happened :-(

Full Circle Talkback ID TB23161948X

Comment 1

[Vincent Deconinck]

A few more precisions (just reproduced the crash to see) :

The table actually gets deleted, and the crash occurs after one or two seconds.

Here's also some more information from the XP "fatal error" message box details :
AppName: mozilla.exe	 AppVer: 1.5.20030.17168	 ModName: editor.dll
ModVer: 1.5.20030.17168	 Offset: 00005f7b

Comment 2

[Robert Accettura [:raccettura]]

I see the same thing.  TB23172642K

Would confirm if I could ;-)

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

Confirming with trunk Seamonkey build 2003-08-29-04 on Windows 2000.

Comment 4

[Robert Accettura [:raccettura]]

Why isn't it CC'ing me by default when I comment.

Comment 5

[Chase Tingley]

See it on linux trunk cvs built 030901

Comment 6

[Chase Tingley]

Attachment: stacktrace

0x420751c1 in nsHTMLEditor::GetCSSBackgroundColorState(int*, nsAString&, int) (

    this=0x89ec6d8, aMixed=0xbfffcf4c, aOutColor=@0xbfffceb0, aBlockLevel=1)
    at nsHTMLEditor.cpp:2467
2467	      res = tmp->GetParentNode(getter_AddRefs(blockParent));

|tmp| is holding the old value of |blockParent|, which is set at line 2460
(since |isBlock| is 0):
2460	      blockParent = GetBlockNodeParent(nodeToExamine);

|nodeToExamine| looks like a valid pointer.

Comment 7

[(not reading, please use [e-mail] instead)]

here's a stack, from by debug build

NTDLL! 77fa144b()
nsDebugImpl::Assertion(nsDebugImpl * const 0x00267008, const char * 0x03c51764
`string', const char * 0x03c517a8 `string', const char * 0x03c517b8 `string',
int 668) line 276
nsDebug::Assertion(const char * 0x03c51764 `string', const char * 0x03c517a8
`string', const char * 0x03c517b8 `string', int 668) line 109
nsCOMPtr<nsIDOMNode>::operator->() line 668 + 34 bytes
nsHTMLEditor::GetCSSBackgroundColorState(int * 0x0012c2e4, nsAString & {...},
int 1) line 2467 + 32 bytes
nsHTMLEditor::GetBackgroundColorState(nsHTMLEditor * const 0x04a74bf0, int *
0x0012c2e4, nsAString & {...}) line 2368 + 24 bytes
nsBackgroundColorStateCommand::GetCurrentState(nsIEditor * 0x04a74b08,
nsICommandParams * 0x0512e018) line 1005 + 46 bytes
nsMultiStateCommand::GetCommandStateParams(nsMultiStateCommand * const
0x0472bd68, const char * 0x0512df88, nsICommandParams * 0x0512e018, nsISupports
* 0x04a74b08) line 682 + 24 bytes
nsControllerCommandTable::GetCommandState(nsControllerCommandTable * const
0x047084c0, const char * 0x0512df88, nsICommandParams * 0x0512e018, nsISupports
* 0x04a74b08) line 226 + 35 bytes
nsBaseCommandController::GetCommandStateWithParams(nsBaseCommandController *
const 0x04a893d4, const char * 0x0512df88, nsICommandParams * 0x0512e018) line 149
XPTC_InvokeByIndex(nsISupports * 0x04a893d4, unsigned int 3, unsigned int 2,
nsXPTCVariant * 0x0012c4e8) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 2017 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x047822a0, JSObject * 0x038e4ff8, unsigned int 2,
long * 0x051161f8, long * 0x0012c7c4) line 1269 + 14 bytes
js_Invoke(JSContext * 0x047822a0, unsigned int 2, unsigned int 0) line 843 + 23
bytes
js_Interpret(JSContext * 0x047822a0, long * 0x0012d5ac) line 2859 + 15 bytes
js_Invoke(JSContext * 0x047822a0, unsigned int 1, unsigned int 2) line 860 + 13
bytes
js_InternalInvoke(JSContext * 0x047822a0, JSObject * 0x015f1228, long 23007880,
unsigned int 0, unsigned int 1, long * 0x0012d808, long * 0x0012d6d8) line 935 +
20 bytes
JS_CallFunctionValue(JSContext * 0x047822a0, JSObject * 0x015f1228, long
23007880, unsigned int 1, long * 0x0012d808, long * 0x0012d6d8) line 3538 + 31 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x048b4060, void * 0x015f1228,
void * 0x015f1288, unsigned int 1, void * 0x0012d808, int * 0x0012d80c, int 0)
line 1217 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x046c60c0, nsIDOMEvent
* 0x05114ff0) line 181 + 77 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x0457a250,
nsIDOMEvent * 0x05114ff0, nsIDOMEventTarget * 0x051150c8, unsigned int 32,
unsigned int 7) line 1194 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x046c6068,
nsIPresContext * 0x03a50420, nsEvent * 0x0012e328, nsIDOMEvent * * 0x0012e254,
nsIDOMEventTarget * 0x051150c8, unsigned int 7, nsEventStatus * 0x0012e350) line
2193 + 36 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x046a5938, nsIPresContext *
0x03a50420, nsEvent * 0x0012e328, nsIDOMEvent * * 0x0012e254, unsigned int 7,
nsEventStatus * 0x0012e350) line 3245
nsXULCommandDispatcher::UpdateCommands(nsXULCommandDispatcher * const
0x03a01af8, const nsAString & {...}) line 387
GlobalWindowImpl::UpdateCommands(GlobalWindowImpl * const 0x047efffc, const
nsAString & {...}) line 3541
XPTC_InvokeByIndex(nsISupports * 0x047efffc, unsigned int 82, unsigned int 1,
nsXPTCVariant * 0x0012e624) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 2017 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x0152f070, JSObject * 0x01657178, unsigned int 1,
long * 0x05106920, long * 0x0012e900) line 1269 + 14 bytes
js_Invoke(JSContext * 0x0152f070, unsigned int 1, unsigned int 0) line 843 + 23
bytes
js_Interpret(JSContext * 0x0152f070, long * 0x0012f6e8) line 2859 + 15 bytes
js_Invoke(JSContext * 0x0152f070, unsigned int 3, unsigned int 2) line 860 + 13
bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x036733a8,
nsXPCWrappedJS * 0x035fbd18, unsigned short 3, const nsXPTMethodInfo *
0x0311e728, nsXPTCMiniVariant * 0x0012fa2c) line 1331 + 22 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x035fbd18, unsigned short 3,
const nsXPTMethodInfo * 0x0311e728, nsXPTCMiniVariant * 0x0012fa2c) line 429
PrepareAndDispatch(nsXPTCStubBase * 0x035fbd18, unsigned int 3, unsigned int *
0x0012fadc, unsigned int * 0x0012facc) line 117 + 31 bytes
SharedStub() line 147
nsCommandManager::CommandStatusChanged(nsCommandManager * const 0x035fb724,
const char * 0x05579058) line 116 + 65 bytes
nsComposerCommandsUpdater::UpdateCommandGroup(const nsAString & {...}) line 309
nsComposerCommandsUpdater::TimerCallback() line 272 + 25 bytes
nsComposerCommandsUpdater::Notify(nsComposerCommandsUpdater * const 0x04a31144,
nsITimer * 0x04aa02e8) line 386
nsTimerImpl::Fire() line 386
nsTimerManager::FireNextIdleTimer(nsTimerManager * const 0x00f83570) line 616
nsAppShell::Run(nsAppShell * const 0x0153a790) line 142
nsAppShellService::Run(nsAppShellService * const 0x0153a718) line 484
main1(int 4, char * * 0x00262638, nsISupports * 0x00ebbe28) line 1290 + 32 bytes
main(int 4, char * * 0x00262638) line 1669 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32!

Comment 8

[(not reading, please use [e-mail] instead)]

I got a patch to bullet proof and prevent the crash, but this bug belongs with
an editor guru (like glazman or akkana)

I'll attach what I got, as we might take it until we have the right fix.

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: wallpaper that prevents the crash (until we get someone like glazman to comment)

Comment 10

[(not reading, please use [e-mail] instead)]

for context:

the code I'm wall papering is from this checkin by glazman:

"CSS in Composer, step 1 ; b=77705, r=jfrancis, r=cmanske, sr=kin"

Comment 11

[(not reading, please use [e-mail] instead)]

daniel, can you review my wall paper, which turns the crash into an assert?

or better yet, feel free to take the bug and fix this the right way.

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Taking bug. This is not specific to mailnews so I change product/component.

Urgent fix needed. Should have one in a few hours (daddy duty
in the meantime)

Comment 13

[Kathleen :Brade]

bummer
Netscape 7.1 / mozilla 1.4 shipped with this bug.  We should try to get this fix
in for 1.4.2 too.

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix #1

Seth: the bug showed by the stack trace should really never happen. It's a bad
side effect of something else, we probably have something weird in the commands

updater. Your patch is really a workaround.

Now, I have a fix for this bug which in my opinion makes sense but I need
the opinion of Kathy Brade and Joe Francis. Why is the current code so complex
in comparison of what I am proposing here ??? Why does it try to update the
commands using an orphan node ? Why (many others).

Comment 15

[(not reading, please use [e-mail] instead)]

yes, worth taking for 1.4.1 if we have a fix

Comment 16

[Robert Accettura [:raccettura]]

I've experienced this several times I think.  I was just never able to pinpoint
what the problem was, as the reporter could.   I've had a few crashes since 1.4~
thanks to tables in email (in retrospect, I realize they all had tables).

So I've lost data in the past.  

And 100.1% agree it should be in 1.4.x.  Isn't that due out soon?  Or is GDI
still holding it up?

Comment 17

[Kathleen :Brade]

Comment on attachment 130924 [details] [diff] [review]
fix #1

This patch looks ok to me; I can't really comment on why it was done that way
(I'd consult with cmanske if he's around).  I'll let Joe check the review+ box.
 r=brade

Do we have a similar problem if we delete a row for a one row table (or
similarly for one-column or one-cell)?

Comment 18

[Kathleen :Brade]

cc cmanske for his input/comments on patch

Comment 19

[Joe Francis]

Daniel, am I to understand your comments to mean that you don't know why your
fix works?  If you do know why it works, can you explain it to me?

Comment 20

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

> Daniel, am I to understand your comments to mean that you don't know why your
> fix works?

Joe: absolutely :-/ I really did not understand why the code crashes with
an orphan node so I wanted to look at the ::DeleteTable code and found it
well hard to follow... I also saw that selecting the Composer and deleting it
using the normal "Delete" menu item in the Edit menu does NOT hit the bug.
My conclusion is that, in the existing code, we try to update the UI at a
wrong moment.

Comment 21

[Kathleen :Brade]

I haven't conclusively tested this but it seems like it is when you paste a
table into the editor and then delete it that you see this crash.  I don't crash
if I create a table, add text to it and then delete it.  Perhaps the problem
lies somewhere in copy/paste (which did undergo some changes before 1.4 shipped).

Joe--if you have time, you could look at the code you wrote for the paste hooks;
maybe we botch something in there (despite having no hook callbacks)?

Comment 22

[Hermann Schwab]

Talkback IDs TB23410495Y, TB23410414Z, reproducible by editing a local file.

For making a testcase, I downloaded a website, and cut it down.
I stripped a lot of stuff, from the start, and from the end, and the file I´m
editing is about the fifth version.

I edited offline (to prevent the flash ad loading), to delete some from the end,
in tagged mode, and crashed.
Then went online to submit the talkback, started mozilla, loaded same file, made
same 3 or 4 deletes in normal mode, and crashed, as seen above.
File and steps to crash available on demand. (I deleted from the end of the file
an immage and a textbox to isolate a flash ad in an iframe.)

Comment 23

[Asa Dotzler [:asa]]

Not very high in the topcrash list. Most comments in talkback talk about editing
or deleting a table in Mail's message compose or Composer. Not going to block
1.5 for this.

Comment 24

[Kathleen :Brade]

I'd really like to see this fix get in soon (at least the wallpaper).

Daniel, have you had a chance to test the "delete row when only 1 row" and
similarly delete column and delete cell?

Comment 25

[Charles Manske]

Attachment: fix #2

The cause of the bug is reseting the selection after the table node is deleted.

My code tried to save the table parent and its offset and reset selection, but
that isn't the right way -- Daniel had the right idea in calling
"SelectTable()" 
in his "fix #1".
This patch is more correct by moving the fix into "DeleteTable2()" so all cases

of deleting tables, such as when deleting a single col or row, are also fixed.
Resulting code simplification removes the necessity of "aSelection" param in
DeleteTable2(). This will be cleaned up in another bug.

Comment 26

[Kathleen :Brade]

r=brade for fix #2
I'll let Joe and/or Daniel set the checkbox. 

Comment 27

[Joe Francis]

Comment on attachment 132542 [details] [diff] [review]
fix #2

looks good, thanks charlie

Comment 28

[Asa Dotzler [:asa]]

This looks like a good fix to get for 1.6a.  Who can help with a super-review?

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 132542 [details] [diff] [review]
fix #2

a=asa (on behalf of drivers) for checkin to 1.6alpha

Comment 30

[Charles Manske]

checked into 1.6a 10-24-03