238413 - element.builder.rebuild() crashes in builds 20040309 and later

Status: RESOLVED INVALID

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

Description

[HJ]

Caillon asked me (http://bugzilla.mozilla.org/show_bug.cgi?id=236946#c7) to file
a new bug for this so I did. 

I keep crashing with element.builder.rebuild() It still works in 20040308 but
fails in all builds with the patch for bug 236946 (cleanup in nsXULElement).
Here's a small code example:

          <menu label="&qpHTTPUserAgentSettings.label;">
            <menupopup id="UAgentMenu" datasources="rdf:null"
ref="urn:useragent-data" 
                       onpopupshowing="initUserAgentMenu(this);">
              <template>
                <rule>
                  <menuitem uri="rdf:*" label="rdf:*" type="radio"/>
                </rule>
              </template>
            </menupopup>
          </menu>

function initUserAgentMenu(aParent)
{
  aParent.builder.rebuild(); <= crash !!!

Comment 1

[HJ]

Steps to reprodue are:

1) install MultiZilla
2) click on QPrefs button
3) hover over 'Browser Spoofing'

Current result : crash
Expected result: no crash

I think this is caused by this change:

-                    return
builder->CreateContents(NS_STATIC_CAST(nsIStyledContent*, unconstThis));
+                    return builder->CreateContents(unconstThis);

Raj, can you please add dr watson/talkback data if they need it, you probably
know that Neil and I can't (read are not allowed to do this kind of actions).

Thanks,
/HJ

Comment 2

[Raj Bhaskar]

Mozilla crashes silently on my WinXP build (2004-03-11-08-trunk) and although it
creates a Dr Watson log in my Win2K build (2004-03-21-08-trunk), I don't think
that Talkbalk was installed (as in, not an option, not that I chose not to
install it).

Comment 3

[Raj Bhaskar]

Attachment: Dr Watson log

In case it's any use, I've attached the Dr Watson stack dump.  As I said in
comment 2, Talkback was not part of my build, so I can't provide an ID

Comment 4

[Raj Bhaskar]

Someone from the MozillaZine forums reproduced the MultiZilla crash and reported
talkback ID TB4559G captured at: 3/24/2004 4:33 PM

Comment 5

[HJ]

Christopher, I'm reassigning this bug to you, because we think that it was your
patch that caused this crasher bug. Can you please fix this bug, or at least
backout the lines that caused this bug?

Comment 6

[HJ]

-> CrashWeek Effort

Comment 7

[Neil Pryde]

I just used HJ's bugzilla account because of this message: 

" You tried to change the Severity field from major to critical, but only the
owner or submitter of the bug, or a sufficiently empowered user, may change that
field."

Comment 8

[HJ]

mozilla 20040309 still crashes for me on WinXP. I will add a testcase that
generated a file called: "bug-238413.rdf" in your profile directory. The content
of that file looks like this:

<?xml version="1.0"?>
<RDF:RDF xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
         xmlns:MZ="http://multizilla.mozdev.org/rdf#">
<RDF:Seq RDF:about="urn:useragent-data">
<RDF:li>Line one</RDF:li>
<RDF:li>Line two</RDF:li>
<RDF:li>Line three</RDF:li>
<RDF:li>Line four</RDF:li>
<RDF:li>Line five</RDF:li>
</RDF:Seq>
</RDF:RDF>

Loading the XUL file (from chrome) should display: "start, adding datasource,
rebuilding and done" but crashes in 20040308+ builds.

Comment 9

[HJ]

Attachment: xul text case (can only be loaded from chrome)

Comment 10

[HJ]

Christopher, anything else that we can do to help?

Comment 11

[HJ]

Jan, can you please have a look at this? This should really be fixed for mozilla
1.7 final. We really need this for work. We're stuck with old mozilla builds
now, because of a serious security issues in prior builds (not this bug). It
would be very nice if you could gear this up a bit, thanks.

Comment 12

[Jan Varga [:janv]]

From bug 236946:

Index: content/xul/content/src/nsXULElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v
retrieving revision 1.512
diff -p -u -d -8 -r1.512 nsXULElement.cpp
--- content/xul/content/src/nsXULElement.cpp	6 Mar 2004 00:22:02 -0000	1.512
+++ content/xul/content/src/nsXULElement.cpp	9 Mar 2004 21:18:18 -0000

// XXX hack because we can't use "mutable"
nsXULElement* unconstThis = NS_CONST_CAST(nsXULElement*, this);

nsCOMPtr<nsIXULTemplateBuilder> builder;

- return builder->CreateContents(NS_STATIC_CAST(nsIStyledContent*, unconstThis));
+ return builder->CreateContents(unconstThis);

from nsIXULTemplateBuilder.idl:
[ptr] native nsIContent_ptr(nsIContent);
[noscript] void createContents(in nsIContent_ptr aElement);
Is is safe to pass |uncostThis| there?

Comment 13

[HJ]

Hmm, InsertElementAt() also fails in these builds but RemoveElementAt() still works.

note: GoogleBox now also crashes as a result of this bug :(

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

It should be just as safe to pass nsXULElement* as it is to pass
nsIStyledContent* to a method which wants nsIContent* since both are unambiguous
descendants of nsIContent*.  I really don't understand why that change would
cause this to break (if it really does).

HJ, a testcase that I can load from non-chrome would be a bigger help.

Comment 15

[HJ]

(In reply to comment #14)
> ... I really don't understand why that change would cause this to break (if it
really does).

Well, I can't find anything else in bonsai for this time time frame, do you?

> HJ, a testcase that I can load from non-chrome would be a bigger help.

Eh, and how should that work? I get a bunch of (security) errors when I try.

All you have to do is to add the attachment to your comm.jar and use
chrome://navigator/content/filename.xul You can simply remove it when you're
done testing. That should do the trick.

Btw, what about comment #4? Is that talkback data any good? 

Comment 16

[Benjamin Smedberg]

(In reply to comment #14)
> It should be just as safe to pass nsXULElement* as it is to pass
> nsIStyledContent* to a method which wants nsIContent* since both are unambiguous
> descendants of nsIContent*.  I really don't understand why that change would

Well, that's theoretically true, but we frequently-enough key hashtables on
nsIContent*, and you would get a different pointer depending on how you do the cast.

Comment 17

[Asa Dotzler [:asa]]

Does this impact the SeaMonkey (or Firefox or Thunderbird) applications?

Comment 18

[HJ]

(In reply to comment #17)
> Does this impact the SeaMonkey (or Firefox or Thunderbird) applications?

Yes and no. Yes, if you add extensions like MultiZilla,but there might be others
having the same kind of difficulties.

Asa, this is one of the issues that keeps us from having a MultiZilla version
for Mozilla Firefox, so yes, it would be nice if this could be fixed for Mozilla 1.8

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

HJ, could you revert the part of the patch Jan mentioned in comment 12 and let
me know if that helps?

Comment 20

[HJ]

(In reply to comment #19)
> HJ, could you revert the part of the patch Jan mentioned in comment 12 and let
> me know if that helps?

No I can't, I wich I could. Also, it still crashes like before.

Comment 21

[HJ]

s/wich/wish

Note to self: open your eyes, for real, and have breakfast first, before adding
comments at 6:45AM ;)

Comment 22

[HJ]

Can someone please make that change so that I can test it?

Comment 23

[Wayne Mery (:wsmwk)]

probably worth retesting. 
Note HJ's email address is dead.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has severity 'critical'.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The code has been rewritten a lot. The function calls mentioned in the comments above were no longer existing. Close as Invalid.