275232 - [FIX]The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup - FFTrunk [@JS_CloneFunctionObject] [@ libmozjs.so]

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Riven Cui]

USER-AGENT: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6)
Gecko/20041217 Firefox/1.0+
BUILD:

The theme "Plastikfox Crystal SVG 1.5.1" downloaded from Mozilla update doesn't
work with this trunk build. But works well with Firefox 1.0.

When choose this theme and restart firefox, and 0xc0000005 error occured in 
js3250.dll.

Comment 1

[Nickolay_Ponomarev]

hmm, bug 273423 says you can't install themes at all :/
Do you have a talkback id?

Comment 2

[Riven Cui]

It's an upgrade installation from firefox 1.0, so all themes are preserved.

Other themes worked OK, except this one.

Where can I find my talkback id?

Comment 3

[Nickolay_Ponomarev]

Did talkback window pop up when you crashed? Do you have it installed?
After you sent the report, it's ID is shown in the Talkback window. Some
information about talkback agent is available in mozillazine's knowledge base. I
don't have an URL now, but the article is called Talkback and the kb's main page
is http://kb.mozillazine.org/

Comment 4

[Riven Cui]

It doesn't crash now, but no window appears when I tried to start it.
In the process list, I can see one firefox.exe process, but it obviously 
has some problem which prevents it to open its main window.

Can I start the feedback agent alone without starting firefox?

Comment 5

[Riven Cui]

Oh, yes. I've found it:
TB2615260K, TB2615200Z

Comment 6

[Nickolay_Ponomarev]

Stack Signature	 JS_CloneFunctionObject cb93f524
Product ID	FirefoxTrunk
Build ID	2004121706
Trigger Time	2004-12-17 22:06:41.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	js3250.dll + (00004167)
URL visited	
User Comments	
Since Last Crash	0 sec
Total Uptime	0 sec
Trigger Reason	Access violation
Source File, Line No.
c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line 3084
Stack Trace 	
JS_CloneFunctionObject 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line 3084]
nsXBLBinding::ExecuteAttachedHandler 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/xbl/src/nsXBLBinding.cpp,
line 850]
nsCSSFrameConstructor::ContentInserted 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 8995]
PresShell::InitialReflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp,
line 2721]
nsXULDocument::StartLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULDocument.cpp,
line 2157]
nsXULDocument::ResumeWalk 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULDocument.cpp,
line 2978]
nsXULDocument::OnStreamComplete 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULDocument.cpp,
line 3219]
nsStreamLoader::OnStopRequest 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/netwerk/base/src/nsStreamLoader.cpp,
line 137]
nsJARChannel::OnStopRequest 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/modules/libjar/nsJARChannel.cpp,
line 689]

Comment 7

[Nickolay_Ponomarev]

The another bug with the same/similar stack trace is bug 264263. Are you sure it
is related to your theme?

Comment 8

[Riven Cui]

I think it's related to the theme.

Because if I don't use this theme (Or use other themes),
firefox will starup without any error.

Similar stack trace maybe caused by similar javascript processing error.

I have no idea, maybe you can contact the author of the theme.

Comment 9

[Riven Cui]

Here is the home page of the theme:
http://www.polinux.upv.es/mozilla/

Please contact the author  V¨ªctor Fern¨¢ndez Mart¨ªnez.  
Email: [e-mail]

Comment 10

[Boris Zbarsky [:bzbarsky]]

The stack in comment 6 is probably bogus -- line 850 of nsXBLBinding.cpp is
where the function ExecuteAttachedHandler returns.  This function does not call
JS_CloneFunctionObject directly (or even via an inlined method).

The line claimed to be crashing is:

  if (OBJ_GET_CLASS(cx, funobj) != &js_FunctionClass) {

which could class if cx or funobj is dead, I suppose...  but again, I suspect
that part of the stack is just wrong.

Comment 11

[Martijn Wargers (dead)]

Using that theme:
works with 2004-09-10 8:52am Firefox trunk build
Crashes with 2004-09-11 7:44am Firefox trunk build

Bonsai link:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-09-10+08%3A00%3A00&maxdate=2004-09-11+08%3A00%3A00&cvsroot=%2Fcvsroot

Comment 12

[Martijn Wargers (dead)]

The browser.xml file and the globalBindings.xml file of that theme makes current
trunk builds crash.
Removing this line in global.css of that theme:
tabbrowser {
  -moz-binding: url("chrome://global/skin/browser.xml#tabbrowser");
}
and removing this line in browser.css of that theme:
statusbarpanel#statusbar-display {
  -moz-binding: url("chrome://global/skin/globalBindings.xml#statusbarpanel-text");
  padding-left: 0;
}
and the theme doesn't crash anymore.
Also, removing the <constructor> tag and all of it's contents of browser.xml and
globalBindings.xml of the corresponding <binding> doesn't make the theme crash
anymore. 
Just leaving an empty <constructor> tag makes the theme crash again.

Comment 13

[Martijn Wargers (dead)]

So bug 230816 seems to me a likely cause for this regression.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Probably, yes...

Since you have a build with this already installed, could you see exactly what
line we crash on in Execute() and what the local variable values there are?

Are there any errors in the JS console indicating that the binding is malformed,
by chance?

Also, are constructors for both of the bindings involved required?  Or just for
the statusbar?

Comment 15

[Martijn Wargers (dead)]

(In reply to comment #14)
> Since you have a build with this already installed, could you see exactly what
> line we crash on in Execute() and what the local variable values there are?
Sorry, I don't have a debug build at the moment. I hope I have one tomorrow.

> Are there any errors in the JS console indicating that the binding is malformed,
> by chance?
No, not that I know of. 
These are the two files, in case you want to look for yourself:
http://martijn.heelveel.info/test/mozilla/browser.xml
http://martijn.heelveel.info/test/mozilla/globalBindings.xml
 
> Also, are constructors for both of the bindings involved required?  Or just for
> the statusbar?
The crash occurs with constructors for either one of them. Only when I remove
both the theme doesn't crash.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm... bindings with inheritance work in general, and I just tried loading those
bindings in a non-chrome context (with a copy of tabbrowser.xml, basically),
with no crash.

Martijn, do you have time to see what the minimal-ish testcase you can come up
with is?  It'll probably involve eviscerating tabbrowser or statusbarpanel-text...

Comment 17

[Martijn Wargers (dead)]

Attachment: Testcase

This testcase crashes when accessed from the chrome://global/skin/ directory.
When I remove the <constructor> tags of the testcase then it doesn't crash
anymore.
Afaik, Mozilla should ignore javascript from the chrome://global/skin/
directory (as themes are not allowed to execute javascript), so I guess
somewhere in that area is the problem.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that should fix this

Martijn, I think you get the "amazing ability to see to the heart of the issue"
award on this one.

Here's what's going on.  Prior to my checkin, we executed constructors and
destructors for all bindings, including those that weren't allowed to execute
script (the binding in this case being a prime example).  My checkin didn't
change this -- we'd call Execute() on the constructor ProtoImplMethod().  The
problem is that is AllowScripts() is false we never call
InstallImplementation() on the prototype, so never compile the methods.  And
methods use a union to hold either the method text or the compiled method
object.  So we were passing a pointer to some string data into the JS engine as
a function object, which crashed, unsuprisingly.

All this patch does is make sure to not call constructors or destructors of
bindings which aren't allowed to execute scripts.  It ought to fix this crash
(Martijn, could you verify that?), but it'll break this theme, of course (since
those constructors will no longer run).

Comment 19

[Martijn Wargers (dead)]

Yes, the patch fixes the crash, I just tried it in my build.

Comment 20

[Brian Ryner (not reading)]

Hm, could this also be because nsXBLProtoImplAnonymousMethod doesn't initialize
mJSMethodObject to null?  The missing frames in the stack are probably because
of tail-calling optimization (I think that's the name for it)...
JS_CloneFunctionObject is getting called from
nsXBLProtoImplAnonymousMethod::Execute, which bails if mJSMethodObject is null,
but as I said, I believe it's used uninitialized if the method is never compiled.

Fixing it in both places seems appropriate to me, so I'll mark r+sr on this patch.

Comment 21

[Brian Ryner (not reading)]

Actually, changed my mind... if we can fix it only in nsXBLProtoImplMethod.cpp
that would be preferable.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Brian, it's not inited to null because it's part of a union.  See comment 18. 
When we're not compiled, the union is pointing to the method text and
mJSMethodObject should not be accessed at all, since it's bogus.

Now I can move all methods to not using a union anymore, but that seems like
unnecessary memory overhead if we can instead fix the union users (like this
one) to not misuse it.

Comment 23

[Brian Ryner (not reading)]

Ah, so it is.  Do you think it's worth adding explicit state for which of the
union members is in use?  At least #ifdef DEBUG?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Yeah, adding it #ifdef DEBUG and putting in corresponding asserts is a good
idea.  I'll do that.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Fixed.  I filed bug 280089 on the safety stuff bryner asked for.

Comment 26

[Jay Patel [:jay]]

Addint FFTrunk to summary and topcrash keyword for future reference, this has
been a topcrasher for recent FirefoxTrunk builds (there have been crashes with
that stack signature in Firefox 1.0 and Mozilla 1.7x builds, but the stack look
different).