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

RESOLVED FIXED in mozilla1.5beta

Status

()

Core
Editor
--
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Vincent Deconinck, Assigned: glazou)

Tracking

({crash, regression})

Trunk
mozilla1.5beta
crash, regression
Points:
---
Bug Flags:
blocking1.4.1 +
blocking1.5 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: TB23161948X, crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

14 years ago
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
(Reporter)

Comment 1

14 years ago
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

Updated

14 years ago
Keywords: crash, stackwanted
Whiteboard: TB23161948X
I see the same thing.  TB23172642K

Would confirm if I could ;-)
Confirming with trunk Seamonkey build 2003-08-29-04 on Windows 2000.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.5?
Why isn't it CC'ing me by default when I comment.

Comment 5

14 years ago
See it on linux trunk cvs built 030901
OS: Windows XP → All

Comment 6

14 years ago
Created attachment 130790 [details]
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.

Updated

14 years ago
Keywords: stackwanted

Updated

14 years ago
Summary: crash if I delete a table in a compose window → crash [@ nsHTMLEditor::GetCSSBackgroundColorState] deleting table in compose window

Updated

14 years ago
Flags: blocking1.5? → blocking1.5+
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!
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.
Created attachment 130887 [details] [diff] [review]
wallpaper that prevents the crash (until we get someone like glazman to comment)
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"
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.
Whiteboard: TB23161948X → TB23161948X [bulletproof crash in hand, but real fix still needed]
(Assignee)

Comment 12

14 years ago
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)
Assignee: sspitzer → daniel
Component: Composition → Editor: Core
Product: MailNews → Browser

Comment 13

14 years ago
bummer
Netscape 7.1 / mozilla 1.4 shipped with this bug.  We should try to get this fix
in for 1.4.2 too.
Flags: blocking1.4.x?
Keywords: regression
Target Milestone: --- → mozilla1.5beta
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 14

14 years ago
Created attachment 130924 [details] [diff] [review]
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).
yes, worth taking for 1.4.1 if we have a fix
Flags: blocking1.4.x? → blocking1.4.x+
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

14 years ago
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

14 years ago
cc cmanske for his input/comments on patch
Hardware: PC → All

Comment 19

14 years ago
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?
(Assignee)

Comment 20

14 years ago
> 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

14 years ago
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

14 years ago
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

14 years ago
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.
Flags: blocking1.5+ → blocking1.5-

Comment 24

14 years ago
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

14 years ago
Created attachment 132542 [details] [diff] [review]
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.
Attachment #130924 - Attachment is obsolete: true

Comment 26

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

Comment 27

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

looks good, thanks charlie
Attachment #132542 - Flags: review+

Updated

14 years ago
Attachment #132542 - Flags: superreview?(kinmoz)

Updated

14 years ago
Whiteboard: TB23161948X [bulletproof crash in hand, but real fix still needed] → TB23161948X
(Reporter)

Updated

14 years ago
Flags: blocking1.6a?

Comment 28

14 years ago
This looks like a good fix to get for 1.6a.  Who can help with a super-review?
Attachment #132542 - Flags: superreview?(kinmoz) → superreview+

Comment 29

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

a=asa (on behalf of drivers) for checkin to 1.6alpha
Attachment #132542 - Flags: approval1.6a+

Comment 30

14 years ago
checked into 1.6a 10-24-03
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Flags: blocking1.6a?
Crash Signature: [@ nsHTMLEditor::GetCSSBackgroundColorState]
You need to log in before you can comment on or make changes to this bug.