Closed
Bug 217717
Opened 21 years ago
Closed 21 years ago
crash [@ nsHTMLEditor::GetCSSBackgroundColorState] deleting table in compose window
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla1.5beta
People
(Reporter: vincent.deconinck, Assigned: glazou)
Details
(Keywords: crash, regression, Whiteboard: TB23161948X)
Crash Data
Attachments
(3 files, 1 obsolete file)
7.25 KB,
text/plain
|
Details | |
809 bytes,
patch
|
Details | Diff | Splinter Review | |
1.51 KB,
patch
|
mozeditor
:
review+
dbaron
:
superreview+
asa
:
approval1.6a+
|
Details | Diff | Splinter Review |
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•21 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•21 years ago
|
Keywords: crash,
stackwanted
Whiteboard: TB23161948X
Comment 2•21 years ago
|
||
I see the same thing. TB23172642K Would confirm if I could ;-)
Comment 3•21 years ago
|
||
Confirming with trunk Seamonkey build 2003-08-29-04 on Windows 2000.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.5?
Comment 4•21 years ago
|
||
Why isn't it CC'ing me by default when I comment.
Comment 6•21 years ago
|
||
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•21 years ago
|
Keywords: stackwanted
Updated•21 years ago
|
Summary: crash if I delete a table in a compose window → crash [@ nsHTMLEditor::GetCSSBackgroundColorState] deleting table in compose window
Updated•21 years ago
|
Flags: blocking1.5? → blocking1.5+
Comment 7•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Comment 10•21 years ago
|
||
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•21 years ago
|
||
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.
Updated•21 years ago
|
Whiteboard: TB23161948X → TB23161948X [bulletproof crash in hand, but real fix still needed]
Assignee | ||
Comment 12•21 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•21 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.
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•21 years ago
|
||
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•21 years ago
|
||
yes, worth taking for 1.4.1 if we have a fix
Flags: blocking1.4.x? → blocking1.4.x+
Comment 16•21 years ago
|
||
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•21 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 19•21 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•21 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•21 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•21 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•21 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•21 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•21 years ago
|
||
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•21 years ago
|
||
r=brade for fix #2 I'll let Joe and/or Daniel set the checkbox.
Comment 27•21 years ago
|
||
Comment on attachment 132542 [details] [diff] [review] fix #2 looks good, thanks charlie
Attachment #132542 -
Flags: review+
Updated•21 years ago
|
Attachment #132542 -
Flags: superreview?(kinmoz)
Updated•21 years ago
|
Whiteboard: TB23161948X [bulletproof crash in hand, but real fix still needed] → TB23161948X
Reporter | ||
Updated•21 years ago
|
Flags: blocking1.6a?
Comment 28•21 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•21 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•21 years ago
|
||
checked into 1.6a 10-24-03
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Flags: blocking1.6a?
Updated•13 years ago
|
Crash Signature: [@ nsHTMLEditor::GetCSSBackgroundColorState]
You need to log in
before you can comment on or make changes to this bug.
Description
•