380872 - Compose (e.g. Right click -> send page) crashes [@ FindBodyContent()]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Bengt-Erik Soderstrom]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070516 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070516 SeaMonkey/1.5a

Open any URL. Right click-> send page. Crash!

Reproducible: Always

Steps to Reproduce:
1. Open any URL
2. Right click and then click send page
3. Crash
Actual Results:  
Crash 

Expected Results:  
Composer window should open and no crash should occur

This happens with the nightly build 2007051601. See talkback incidence TB32193615Q

Comment 1

[Adam Guthrie]

I'm assuming that this is a recent regression?

Incident ID: 32193615
Stack Signature	FindBodyContent() 30452eec
Product ID	MozillaTrunk
Build ID	2007051601
Trigger Time	2007-05-16 06:27:24.0
Platform	LinuxIntel
Operating System	Linux 2.6.20-1.2948.fc6
Module	libgklayout.so + (0022b003)
URL visited	any page
User Comments	Right click -> send page. Crash!
Since Last Crash	0 sec
Total Uptime	0 sec
Trigger Reason	SIGSEGV: Segmentation Fault: (signal 11)
Source File, Line No.	/builds/tinderbox/SeaMonkey-Trunk/Linux_2.6.9-42.ELsmp_Depend/mozilla/objdir/layout/xul/base/src//builds/tinderbox/SeaMonkey-Trunk/Linux_2.6.9-42.ELsmp_Depend/mozilla/layout/xul/base/src/nsListBoxObject.cpp, line 172
Stack Trace 	
FindBodyContent()   FindBodyContent()   nsListBoxObject::GetListBoxBody()   nsListBoxObject::GetIndexOfFirstVisibleRow()   NS_InvokeByIndex_P()
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)()   XPC_WN_CallMethod()   js_Invoke()   js_Interpret()   js_Execute()   JS_EvaluateUCScriptForPrincipals()   nsJSContext::EvaluateString()   nsGlobalWindow::RunTimeout()   nsGlobalWindow::TimerCallback()   nsTimerImpl::Fire()   nsTimerEvent::Run()   nsThread::ProcessNextEvent()   NS_ProcessNextEvent_P()  [mozilla/objdir/xpcom/build/nsThreadUtils.cpp, line 227]
nsBaseAppShell::Run()   nsAppStartup::Run()   main1()   main()   libc.so.6 + 0x15f2c (0x00991f2c)

Comment 2

[Mats Palmgren (inactive)]

Attachment: stack

GetDocument() on the xul:listrows child returns null.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsListBoxObject.cpp&rev=1.24&root=/cvsroot&mark=164,172,190,207#163

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip

Not sure what the right fix is here, but this doesn't crash at least.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So I'm a little confused.  frame->GetContent()->GetDocument() is not null?  But the document of one of the xbl child nodes _is_ null?  How can that happen?

For what it's worth, I think it would make the most sense to get the binding manager off the ownerdocument, but the above inconsistency seriously worries me.

I also wonder whether this would fix the non-crash parts of bug 380866.

Comment 8

[Jerry Baker]

Simply opening a blank message compose window in Thunderbird crashes with the same stack. Changing OS to all and severity to blocker since composing messages is now completely disabled.

Comment 9

[Jerry Baker]

Should this be moved to MailNews:Composition since the "Send Page" thing is not necessary to reproduce this, and it happens in Thunderbird as well? 

Comment 10

[Peter Reynolds]

One can see a load of Talkback crashes connected with this <a href="http://talkback-public.mozilla.org/search/start.jsp?search=1&searchby=stacksig&match=contains&searchfor=FindBodyContent&vendor=MozillaOrg&product=ThunderbirdTrunk&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid">here</a>

Comment 11

[Mats Palmgren (inactive)]

Investigating...

Comment 12

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

I'm pretty sure I know what the problem is and I'm working on a patch.

Comment 13

[Mats Palmgren (inactive)]

Ok, thanks.
Just to answer Boris' questions (comment 5) from memory:
Yes. Yes. Don't know.  As I recall, changing FindBodyContent()
to use GetOwnerDocument() also fixed it.

Comment 14

[Fernando Piera]

This bug is also for thunderbird standalone operating with windows 2000 professional and with trunk version 20070517

Comment 17

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

Attachment: Will hopefully do it.

This should call BindToTree on all anonymous children that we need to do it on. I don't have a thunderbird build so I haven't been able to reproduce the problem, would be great if someone else could try it out.

Comment 18

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

Comment on attachment 265323 [details] [diff] [review]
Will hopefully do it.

bz: so I think what is happening is that we have anonymous content bound to one of these evil clones, and then the clone is attached to the tree and rendered.

This used to work fine since the evil clone had in-doc set, so the in-doc flag propagated to the anonymous content.

However now that in-doc is not set on the evil clone, it won't be on the anonymous content either. When the clone is inserted into the tree we don't call BindToTree on the anonymous content, and so it remains "out of doc".

I hope I got the patch right, it feels like the right thing to do either way.

Would be especially great if you could confirm that the parameters given to BindToTree makes sense.

Comment 19

[Mats Palmgren (inactive)]

I can confirm that the patch fixes the crash in SeaMonkey for me.
I get an assertion on exit though, although it might be unrelated:

###!!! ASSERTION: attempt to remove an element that was never added: 'hep != nsnull && *hep != nsnull', file nsElementMap.cpp, line 281

Comment 20

[Boris Zbarsky [:bzbarsky]]

I need to do some digging to review this; with any luck I can do it by Sunday.

Comment 21

[Phil Ringnalda (:philor)]

Stops Thunderbird from crashing, which is  nice, but the address autocomplete
widget is pretty completely broken.

Opening compose gives

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMXULElement.boxObject]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/textbox.xml ::  :: line 117"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMXULElement.boxObject]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/autocomplete.xml :: set_view :: line 1497"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMXULElement.boxObject]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/menulist.xml :: <TOP_LEVEL> :: line 125" 
data: no]

and the address widget's only maybe a quarter of the window's width. Start
typing an address, and you get an empty autocomplete popup the same too-small
width, and mousing over the empty space gives a stream of 

Error: this.textbox.view.treeBoxObject has no properties
Source File: chrome://global/content/autocomplete.xml
Line: 1667

Comment 22

[Frederic Bezies]

erh...

With latest updated version (20 of may), my thunderbird is *still* crashing when clicking on "new message" icon on toolbar.

Seeing it with Windows Vista.

So it is a windows "crappysta" bug ?!

Comment 23

[Michael Mak]

It's effecting Linux, Windows 2000 and Vista. I think its safe to say its effecting all OS. See previous comments.

Comment 24

[David :Bienvenu]

this patch stops the crash, and you can see the address widget, but the display of address you type or autocomplete to is clipped after about 20 chars. Maybe that's just something wrong with my build, but it's something to look for.

Comment 25

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

Yeah, i know that problem is still there. I'll land this patch tomorrow (assuming i get review) and look into what that is. If I can't fix the remaining problem pretty quickly I'll back out the original cause of these regressions.

Comment 27

[Peter Reynolds]

I notice from the comments above that there seem to be several separate problems here (a) it crashes when you try to write a message,(b) there is nothing in the address area, (c) even when the above is fixed there is a problem with the address area being limited to about 20 characters.  

Can't whatever changes were made a few days ago be regressed until the people who made them have working code that doesn't cause the system not to work?  I know the trunk nightly is an Alpha, but how did an upgrade which caused something as basic as composing messages not to work even get into Alpha?

Comment 28

[Robert Kaiser]

IMHO, the crash fix should go in fast (I hope bz comes around to reviewing it soon), then there should be a followup filed or multiple followups filed for remaining issues, if they are not fixed by the fix here.

Peter:
Trunk is not even Alpha, it's bleeding-edge current development code. Neither Thunderbird nor SeaMonkey have released an Alpha based on Gecko 1.9 trunk yet.

Comment 29

[Peter Reynolds]

Maybe we're at cross purposes here.  My Thunderbird says it is "version 3 alpha 1" followed by a version/date number in brackets.

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to comment #29)
> My Thunderbird says it is "version 3 alpha
> 1" followed by a version/date number in brackets.

As Robert wrote, it is not officially _released_:
the _nightly_, which I guess you use, only tells you what (goal) is being worked on...

Comment 31

[Michael Mak]

In Firefox (aka Minefield) they are using Minefield 3.0a5pre, which means the goal is Alpha 5.

Maybe we should have the same thing in Thunderbird? I'm surprised Thunderbird is not using this already. Although this is a separate issue. Should I file a new bug for this?

Comment 32

[Serge Gautherie (:sgautherie)]

(Yes, it's TB (Trunk) issue. Searching/Filing a bug should do no harm.)

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265323 [details] [diff] [review]
Will hopefully do it.

>Index: content/base/src/nsGenericElement.cpp
> nsGenericElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,

>+        nsCOMPtr<nsIContent> anonRoot = contBinding->GetAnonymousContent();

I assume the strong ref is just in case?  Seems reasonable.

>+      // ...then check if we have content in insertion points
>+      if (aBindingParent) {
>+        nsXBLBinding* binding = bmgr->GetBinding(aBindingParent);
>+        NS_ASSERTION(binding, "huh, no binding?");

I'm not sure you can make this assertion.  For example, |this| could be a Text node in a textarea, and aBindingParent would be the anonymous <div> inside the textarea, no?

>+                  nsCOMPtr<nsIContent> child = insertRoot->GetChildAt(j);

Again, I assume this is a just in case?

This looks reasonable... I should have told you to test tbird with the other patch; I recall now that the composition UI is a continuous XBL trouble spot.  :(  Sorry about that.

Comment 37

[Fernando Piera]

New nightly update 20070521 09 still crashed when trying to write a message

Comment 38

[Michael Mak]

(In reply to comment #32)
> (Yes, it's TB (Trunk) issue. Searching/Filing a bug should do no harm.)
> 

https://bugzilla.mozilla.org/show_bug.cgi?id=381418 Bug filed and patch should appear in the next nightly.

Michael

Comment 39

[Michael Mak]

(In reply to comment #37)
> New nightly update 20070521 09 still crashed when trying to write a message
> 

It landed after the nightly (20070521) was released. It should work with the latest hourly and the next nightly build (0522)

Comment 40

[Howard Chu]

Attachment: addressingWidget patch

I just found this bug report. Was running into the same issues described above - composing a new message would crash; replying to a message would work but the addressingWidget wouldn't display. The error console showed 3 uncaught exceptions (already noted in above comments) because in CompFields2Recipients() the newListBoxNode didn't have a valid boxObject.

I came up with this quick hack of appending newListBoxNode to the listbox's parent first to avoid the exceptions. Then popping it off again so it doesn't get in there twice at the end. Just for reference; if the above patch works you can ignore this.

Comment 41

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

There still seems to be some nodes that don't get there IsInDoc flag set as needed, so lets try to fix that before creating workarounds all over. Feel free to file separate bugs on unrelated issues that you find though, of course.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.   I guess GetBoxObjectFor() bails out if GetCurrentDoc() != this.  We might need to hack this if the XBL bit is set, perhaps, if XBL constructors/fields/whatever sue the box object.  And we'd need a way to deal with bug 330181, maybe.  But it would be no worse than when nodes lied about their document, so...

Comment 43

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

Attachment: Fix insertion points directly below <content>

If there is a <children> as a direct child of <content> the bound element can have insertion points in its binding. This should take care of such insertion points as well.

I'm still getting some layout weirdness, and some assertions about the XUL-document element map, but at least with this patch we get into a dogfoodable state while I keep looking for the remaining regressions.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265596 [details] [diff] [review]
Fix insertion points directly below <content>

>Index: content/base/src/nsGenericElement.cpp
>+BindNodesInInsertPoints(nsXBLBinding* aBinding, nsIContent* aInsertParent,
>+                        nsIDocument* aDocument, PRBool aAllowScripts)

I'd get rid of the aAllowScripts arg and just use aBinding->AllowScripts().  If you really feel this is faster or something (which I doubt), at least assert that the two are equal.

Also, assert that aBinding and aInsertParent are not null?

r+sr=bzbarsky with that.

Comment 46

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

Yeah, I at first used aBinding->AllowScripts, but the implementation of AllowScripts looked big and bulky. But I suspect you're right that it's fast enough so I'll use that.

Comment 47

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/2007052204 SeaMonkey/1.5a] (suiterunner, tinderbox-builds) (W2Ksp4)

Simple confirmation that the Compose crash part is fixed.

Comment 48

[Fernando Piera]

tested the new nightly update 2007052204.  Thunderbird standalone does not crash but in the compose area still the address space filling has problems. In reply to all the detailed addresses sometime are cut in half missing the last past.  The same in the reply where the address maybe cut or missing a piece of it.
When writing a message, the address preestablished do not appear complete, the box now is too narrow to contain the full lists of address that match with the first letters introduced. Should I fill a new bug, or it not necessary?

Comment 49

[Fernando Piera]

tested the new nightly update 2007052204.  Thunderbird standalone does not crash but in the compose area still the address space filling has problems. In reply to all the detailed addresses sometime are cut in half missing the last past.  The same in the reply where the address maybe cut or missing a piece of it.
When writing a message, the address preestablished do not appear complete, the box now is too narrow to contain the full lists of address that match with the first letters introduced. Should I fill a new bug, or it not necessary?

Comment 50

[Serge Gautherie (:sgautherie)]

Fernando,
that is dependent bug 381553 !

Comment 51

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

Attachment: Fix more issues

The new AddSubtreeToDocument call fixes some assertions I was seeing and seems like the right thing to do.

The GetBoxObject changes fixes some warnings, not sure if they are strictly needed or not, but lets do this until we've fixed all regressions at least.

The nsNodeUtils.cpp change fixes the sizing problem of the address widget. It seems really unfortunate, but I'd like to investigate why it's needed after we've fixed the regressions.

Still having issues with the dropdown, but this should at least get us to a better state.

Comment 52

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

Attachment: Fix more issues

Fixes a compile error in previous patch.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265867 [details] [diff] [review]
Fix more issues

>Index: content/base/src/nsDocument.cpp
>+  nsIDocument* doc = content->HasFlag(NODE_FORCE_XBL_BINDINGS) ?
>+    content->GetOwnerDoc() : content->GetCurrentDoc();

We should file a followup bug to sort out how box objects should work... again.  We made this use the current document because of security issues in the first place; of course those issues remained with XUL.  :(

>Index: content/base/src/nsGenericElement.cpp

>+            xulDoc->AddSubtreeToDocument(child);

Nice catch.

>Index: content/base/src/nsNodeUtils.cpp
>+      /* For elements that have the NODE_FORCE_XBL_BINDINGS flag  \
>+         set we need to notify the document */                    

Ugh.  Again, followup bug?  I _really_ want to know what depends on this!

Thanks for digging into this, Jonas!

r+sr=bzbarsky

Comment 54

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

The only remaining issue that I'm seeing is that the dropdown with completations is only a single row high. This issue is fixed by Howards patch in attachment 265569 [details] [diff] [review].

Howard, can you explain what that patch does. Do you know why it's needed? What fails if we don't do that?

Comment 55

[Joe Sabash [:JoeS1]]

With the patch the dropdown on autocompete does show more than one row.
Winxp sp2 but horizontal and vertical scrollbars are rendered.
See: http://img406.imageshack.us/img406/6907/addpanemu5.gif

Comment 56

[Joe Sabash [:JoeS1]]

The patch I was referring to was not Howard's but just with your's Jonas.
test via houry Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070524 Thunderbird/3.0a1pre ID:2007052417 [cairo]

Comment 57

[Bengt-Erik Soderstrom]

I just filed bug 381826 which might be a connected to this bug. (Mail sorting and filtering not working)

Comment 58

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070525 SeaMonkey/1.5a] (nightly) (W2Ksp4)

(In reply to comment #55)
> With the patch the dropdown on autocompete does show more than one row.

I don't know which patch...
But I confirm that the dropdown (now) shows multiple addresses :-)

> Winxp sp2 but horizontal and vertical scrollbars are rendered.

Vertical sb is expected.
Horizontal sb is a regression :-(

Comment 59

[Joe Sabash [:JoeS1]]

(In reply to comment #58)
> [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070525
> SeaMonkey/1.5a] (nightly) (W2Ksp4)
> 
> (In reply to comment #55)
> > With the patch the dropdown on autocompete does show more than one row.
> 
> I don't know which patch...
> But I confirm that the dropdown (now) shows multiple addresses :-)
> 
> > Winxp sp2 but horizontal and vertical scrollbars are rendered.
> 
> Vertical sb is expected.
> Horizontal sb is a regression :-(
> 


Submitted bug 382083 to track this last remaining regression.

Comment 60

[sekhar]

Attachment: t

Comment 61

[Scott MacGregor]

Comment on attachment 265569 [details] [diff] [review]
addressingWidget patch

This patch from Howard makes the horizontal scrollbar in the popup go away for me. Jonas and I were talking about this patch and we'd like to take it.

I think Neil knows this code best though, asking him for a review.

Comment 62

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

Comment on attachment 265569 [details] [diff] [review]
addressingWidget patch

>@@ -213,6 +213,8 @@
>     var msgRandomHeaders = msgCompFields.otherRandomHeaders;
>     var msgNewsgroups = msgCompFields.newsgroups;
>     var msgFollowupTo = msgCompFields.followupTo;
>+    var parent = listbox.parentNode;
>+    parent.appendChild(newListBoxNode);

Do you want to make this
parent.appendBefore(newListBoxNode, listbox);

>     // dump("replacing child in comp fields 2 recips \n");
>-    var parent = listbox.parentNode;
>+    parent.removeChild(newListBoxNode);
>     parent.replaceChild(newListBoxNode, listbox);
>     awFitDummyRows(2);

... and this parent.removeChild(listbox) to save yourself some shuffling of the nodes?

Comment 63

[Serge Gautherie (:sgautherie)]

(In reply to comment #40)
> Created an attachment (id=265569) [details]
> addressingWidget patch
> 
> I came up with this quick hack of appending newListBoxNode to the listbox's
> parent first to avoid the exceptions. Then popping it off again so it doesn't

Is that (still) the right fix ?
The 3 exceptions have been fixed already.
Now, doesn't this look too much like a hack rather than a fix ?

NB: While there, could you add a space after '//' ?

Comment 64

[[e-mail]]

Comment on attachment 265569 [details] [diff] [review]
addressingWidget patch

>+    var parent = listbox.parentNode;
>+    parent.appendChild(newListBoxNode);

>-    var parent = listbox.parentNode;
>+    parent.removeChild(newListBoxNode);
>     parent.replaceChild(newListBoxNode, listbox);
This is way too complicated. Just move both lines.
Also I'd prefer them moved to just after the var templateNode line.

Comment 65

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

Attachment: Fix review comments

Neil: Does this look better?

Re comment 63: I don't believe this is a hack, XUL generally don't work when used outside of the document since XBL only binds to nodes inside the document, this is a known problem and has always existed. The fact that this code ever worked is due to a big and problematic hack involving cloned XUL nodes.

When I first fixed (part of) that hack we got some scary inconsistencies that I've fixed in the initial patches in this bug.

With that fixed cloned XUL elements now behave more like normal XUL elements. And since this widget seems to be the only one affected by this so far, I'd rather change the widget than try to add back more hacks into XUL.

Comment 66

[Serge Gautherie (:sgautherie)]

(All right, I understand.
The new patch "looks" much better than the previous, though.)

Comment 67

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

Comment on attachment 266662 [details] [diff] [review]
Fix review comments

Got sr=bienvenu over irl

Comment 68

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

Waiting to get Neils official r before closing this one.

Comment 69

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/2007053102
SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

You can count this very comment as a
"V.Fixed, as all blockers are now fixed."
when you resolve this bug.

Comment 70

[Boris Zbarsky [:bzbarsky]]

> The nsNodeUtils.cpp change fixes the sizing problem of the address widget. It
> seems really unfortunate, but I'd like to investigate why it's needed after
> we've fixed the regressions.

Bug 398492 is a crash regression from this change.  It has some thoughts on why it was maybe needed and how to get rid of it.