Closed Bug 275232 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: XBL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: melop_cui, Assigned: bzbarsky)

References

()

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(2 files)

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.
hmm, bug 273423 says you can't install themes at all :/
Do you have a talkback id?
Keywords: aviary-landing
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?
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/
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?
Oh, yes. I've found it:
TB2615260K, TB2615200Z
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]
Summary: The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup → The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup [@JS_CloneFunctionObject]
The another bug with the same/similar stack trace is bug 264263. Are you sure it
is related to your theme?
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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: vfernandez@polinux.upv.es
Assignee: firefox → hyatt
Component: General → XBL
Product: Firefox → Core
QA Contact: firefox.general → ian
Severity: normal → critical
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.
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.
So bug 230816 seems to me a likely cause for this regression.
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?
(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.
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...
Attached file 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.
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).
Assignee: hyatt → bzbarsky
Status: NEW → ASSIGNED
Attachment #171957 - Flags: superreview?(bryner)
Attachment #171957 - Flags: review?(bryner)
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup [@JS_CloneFunctionObject] → [FIX]The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup [@JS_CloneFunctionObject]
Target Milestone: --- → mozilla1.8beta
Yes, the patch fixes the crash, I just tried it in my build.
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.
Actually, changed my mind... if we can fix it only in nsXBLProtoImplMethod.cpp
that would be preferable.
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.
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?
Attachment #171957 - Flags: superreview?(bryner)
Attachment #171957 - Flags: superreview+
Attachment #171957 - Flags: review?(bryner)
Attachment #171957 - Flags: review+
Yeah, adding it #ifdef DEBUG and putting in corresponding asserts is a good
idea.  I'll do that.
Fixed.  I filed bug 280089 on the safety stuff bryner asked for.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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).
Keywords: topcrash
Summary: [FIX]The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup [@JS_CloneFunctionObject] → [FIX]The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup - FFTrunk [@JS_CloneFunctionObject]
Summary: [FIX]The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup - FFTrunk [@JS_CloneFunctionObject] → [FIX]The theme "Plastikfox Crystal SVG 1.5.1" make this build of firefox crash on startup - FFTrunk [@JS_CloneFunctionObject] [@ libmozjs.so]
Crash Signature: [@JS_CloneFunctionObject] [@ libmozjs.so]
You need to log in before you can comment on or make changes to this bug.