403841 - Crash in [@ nsContentUtils::CreateContextualFragment] when passed a non-element node as context node (SeaMonkey/Thunderbird crashes when sending mail)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andy Boze]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9b2pre) Gecko/2007111202 SeaMonkey/2.0a1pre equivalent to Firefox/2.0.0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9b2pre) Gecko/2007111202 SeaMonkey/2.0a1pre

After composing a new e-mail, or a reply or a forward, when I click on the Send button, SeaMonkey crashes. This doesn't happen 100% of the time, but very close to it. This began with the 20071113 nightly trunk build.

Reproducible: Sometimes

Steps to Reproduce:
1.Compose a new e-mail, or reply to or forward an existing one.
2.Click the Send button
3.
Actual Results:  
Crash!

Expected Results:  
SeaMonkey does not crash.

The e-mail is sent successfully and a copy is saved in my Sent folder. I'm using an IMAP account.

Comment 1

[Karsten Düsterloh]

Do you get a breakpad window (and ID)?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I confirm.  With SM  rv:1.9b2pre) Gecko/2007111403 SeaMonkey/2.0a1pre
Any time you close a plain text mail compose window, either because
a) you sent the mail successfully, or
b) the attempt to send the mail failed, or
c) you closed the window without attempting to send the message,
SM crashes.  Boom, bang, pow, Zap.

I think all these crash IDs are for this problem.

Crash ID: bp-19f40a61-9302-11dc-9615-001a4bd43ed6
You can view details of this crash at http://crash-stats.mozilla.com/report/index/19f40a61-9302-11dc-9615-001a4bd43ed6?date=2007-11-14-22

Crash ID: bp-50d12554-9307-11dc-8899-001a4bd43ef6
You can view details of this crash at http://crash-stats.mozilla.com/report/index/50d12554-9307-11dc-8899-001a4bd43ef6?date=2007-11-14-23

Crash ID: bp-95070d91-934b-11dc-bdc0-001a4bd43e5c
You can view details of this crash at http://crash-stats.mozilla.com/report/index/95070d91-934b-11dc-bdc0-001a4bd43e5c?date=2007-11-15-07

Crash ID: bp-b33e4500-9307-11dc-b489-001a4bd43e5c
You can view details of this crash at http://crash-stats.mozilla.com/report/index/b33e4500-9307-11dc-b489-001a4bd43e5c?date=2007-11-14-23

Comment 4

[Karsten Düsterloh]

Then it's most probably a dupe of bug 403664, because of the stacks.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Well, bug 403664 says the problem is caused by pasting data into the 
window.  I guess that Charles and I should see if we can reproduce 
the problem without pasting.  If not, then I'd agree it's a dup.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I did a test.  I had an unsent message in my drafts folder.  I double-clicked
on it, and then clicked send.  No pasting at all. Bam. Boom. Pow. Oof!  
(reminds me of old Batman comix)

Comment 7

[Charles]

Bug 403664 is a Firefox bug. I don't have to paste to reproduce the bug.

Comment 8

[Andy Boze]

(In reply to comment #1)
> Do you get a breakpad window (and ID)?
> 

Yes. Here are the IDs from yesterday.
bp-b37c8ee3-9309-11dc-a4cf-001a4bd43e5c
bp-a94c7c7d-9307-11dc-9eae-001a4bd43ef6
bp-9d4e1cb8-9305-11dc-8fbf-001a4bd43e5c
bp-9d749c3a-9300-11dc-8856-001a4bd43ef6

Comment 11

[Robert Kaiser]

Reopening, as the fix for bug 403664 didn't fix this crash after all.

I attached a stacktrace there as attachment 288715 [details] and verified that backing out bug 403549 fixes this crash.

Comment 12

[(not currently active) Ted Mielczarek]

bug 403664 was crashing because we were accidentally calling nsParser::ParseFragment with aTagStack being empty.  This caused the call to Parse() to return early here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/parser/htmlparser/src/nsParser.cpp&rev=3.403&root=/cvsroot#1310, which in turn triggered the assertion here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/parser/htmlparser/src/nsParser.cpp&rev=3.403&root=/cvsroot#1460 and then crashed.  You can probably figure out this crash by breaking in nsParser::ParseFragment, and then stepping through or working backwards to figure out what's failing.

Comment 13

[Robert Kaiser]

With my lack of knowledge of C++ or gdb, the only thing I could find out before being frustrated by getting nowhere and giving up was that we indeed end up with an empty aTagStack ending up in nsParser::ParseFragment when being called by http://mxr.mozilla.org/seamonkey/source/content/base/src/nsContentUtils.cpp#3308 as a result of sending a message.

Comment 14

[Samuel Sieb]

The patch from bug 403877 fixes this.

Comment 15

[(not currently active) Ted Mielczarek]

Figured it out. nsHTMLEditor::ReplaceHeadContentsWithHTML is calling nsContentUtils::CreateContextualFragment with the context node being a text node.  sicking's patch changed the semantics of the while loop in that method so that it ends if the node is not an element, so the tag stack doesn't get built.  Patch in a minute.

Comment 16

[(not currently active) Ted Mielczarek]

Attachment: sicking's suggested fix + mochitest

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 289053 [details] [diff] [review]
sicking's suggested fix + mochitest

Thanks!

Comment 18

[(not currently active) Ted Mielczarek]

Checked in.

Comment 19

[Fernando Piera]

I would say that with nightly update version 3.0a1pre (2007111703) TB problems with send, forward emails are resolved and consequently bugs 403841 and 403664 seems solved

Comment 20

[Joe Sabash [:JoeS1]]

Ted didn't set his alarm clock early enough to make the "nightlies"
But seriously Ted, your help is much appreciated by nightly testers.
Too often core bugs span the weekend, which is prime time for many testers.

Comment 21

[Reed Loden [:reed]]

I backed this out as a possible cause of the Ts regression on Linux.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It doesn't make any sense that this caused a regression. It's basically just putting back code that was there before.

Comment 23

[(not currently active) Ted Mielczarek]

Well, it probably does one more call to IsNodeOfType per call to CreateContextualFragment, but if that really causes a Ts hit, then our startup code must be screwed already.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What's more important is that this new code still does way less work than the code did before bug 403549 was originally checked in.

What could be happening here though is that not having this in causes a bug that makes us do less work (since we'll fail to parse things properly).

I'll try to back bug 403549 out and see if that also causes the regression. If so checking this in is simply bringing us back to baseline.

Comment 25

[(not currently active) Ted Mielczarek]

I'm not sure about us doing less work, since that would trigger the crash, wouldn't it?  We could easily rewrite the loop to work more like it used to, something like;
while(content) {
  if(content->IsNodeOfType(nsINode::eELEMENT)) {
    ...
  }
  content = content->GetParent();
}
In which case any perf regression would be entirely due to correctness.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Writing the loop that way should make it behave exactly the same, so there should be no performance difference.

Another fix has been checked in for the crash, so we wouldn't necessarily crash even if we did less work. But it's true that if we ran into this case at startup we would have crashed before that fix was checked in, and we clearly didn't.

So I'm not sure what is going on here. But I think I'll still try to back everything out and then land in one go to see what's going on.

Comment 28

[Robert Kaiser]

Will there be another attempt to this or will we live with bug 403877 papering over this?

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll try to reland

Comment 30

[Reed Loden [:reed]]

(In reply to comment #29)
> I'll try to reland

Did this happen?

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Relanded this and saw not Ts regression.