Closed
Bug 369714
Opened 17 years ago
Closed 17 years ago
Thunderbird crash on startup [@js_HashString()]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ecfbugzilla, Assigned: crowderbt)
Details
(Keywords: fixed1.8.0.12, Whiteboard: [sg:moderate?])
Attachments
(3 files, 3 obsolete files)
77.79 KB,
application/octet-stream
|
Details | |
3.46 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
2.04 KB,
patch
|
igor
:
review+
dveditz
:
approval1.8.0.12+
|
Details | Diff | Splinter Review |
I got a bug report about Thunderbird crashing on startup under certain conditions. These conditions are: Mac OS X 10.3.9, Thunderbird 1.5.0.9, Adblock Plus 0.7.2.4, FlashGot 0.5.97.03 and a specific patterns.ini that will be loaded by Adblock Plus on startup. The crash is stable: TB29095135Z TB29095158G TB29095216X TB29095954K TB29096078K Here is the stack from the talkback reports: js_HashString() [mozilla/js/src/jsstr.c, line 2825] js_AtomizeString() [mozilla/js/src/jsatom.c, line 651] fun_xdrObject() [mozilla/js/src/jsfun.c, line 1350] js_XDRObject() [mozilla/js/src/jsobj.c, line 4323] JS_XDRValue() [mozilla/js/src/jsxdrapi.c, line 544] XDRAtomListElement() [mozilla/js/src/jsscript.c, line 351] XDRAtomMap() [mozilla/js/src/jsscript.c, line 385] js_XDRScript() [mozilla/js/src/jsscript.c, line 519] JS_XDRScript() [mozilla/js/src/jsxdrapi.c, line 579] nsXULPrototypeScript::Deserialize() [mozilla/content/xul/content/src/nsXULElement.cpp, li] nsXULPrototypeScript::DeserializeOutOfLine() [mozilla/content/xul/content/src/nsXULElemen] nsXULPrototypeElement::Deserialize() [mozilla/content/xul/content/src/nsXULElement.cpp, l] nsXULPrototypeDocument::Read() [mozilla/content/xul/document/src/nsXULPrototypeDocument.] nsXULPrototypeCache::GetPrototype() [mozilla/content/xul/document/src/nsXULPrototypeCach] nsChromeProtocolHandler::NewChannel() [mozilla/chrome/src/nsChromeProtocolHandler.cpp, line 842] nsIOService::NewChannelFromURI() NS_NewChannel() [mozilla/mailnews/mime/src/mimecth.cpp, line 172] nsDocShell::DoURILoad() nsDocShell::InternalLoad() nsDocShell::LoadURI() nsDocShell::LoadURI() nsWebShellWindow::Initialize() nsAppShellService::JustCreateTopWindow() nsAppShellService::CreateHiddenWindow() nsAppStartup::CreateHiddenWindow() XRE_main() [mozilla/toolkit/xre/nsAppRunner.cpp, line 2271] _start() start() Not sure this crash can be reproduced but I will attach the patterns.ini file I was sent (should be placed into the adblockplus directory in the profile). Since this seems to depend on the objects that Adblock Plus creates and since js_HashString clearly gets a wrong memory location from somewhere I am marking this security sensitive just to be sure.
Reporter | ||
Comment 1•17 years ago
|
||
Reporter | ||
Updated•17 years ago
|
Attachment #254402 -
Attachment mime type: text/plain → application/octet-stream
![]() |
||
Updated•17 years ago
|
Flags: blocking1.9?
Reporter | ||
Comment 2•17 years ago
|
||
Tried to reproduce this crash, didn't work - neither on Mac OS X nor on Windows. Anybody able to make an educated guess from the stack trace?
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsfun.c&mark=1224,1228,1297,1349&rev=THUNDERBIRD_1_5_0_9_RELEASE#1224 personally, i'd be asking what protects atomstr from dying, especially given that, i think propname could evict it.
Comment 4•17 years ago
|
||
propname is initialized from JS_GetStringBytes, which deflates a string into the deflated string cache, but does not allocate a GC-thing. So atomstr still should be hanging by a thin reed. But it's a long way from where atomstr is created to where it's atomized, so yeah: JS_PUSH_TEMP_ROOT_STRING should be used. Cc'ing folks who should fight to take this bug. /be
Assignee | ||
Comment 5•17 years ago
|
||
I will take this one as soon as I am done with the array bug.
Assignee: general → crowder
OS: Mac OS X → All
Hardware: Macintosh → All
Assignee | ||
Comment 6•17 years ago
|
||
I don't have a working thunderbird build to test this with, so help would be appreciated.
Attachment #256972 -
Flags: review?(brendan)
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 7•17 years ago
|
||
Comment on attachment 256972 [details] [diff] [review] add a root for atomstring Don't add string to JSTempValueUnion. Do consolidate tvr per C function, there's no need for two. Igor should be reviewer of choice on such patches, so I don't delay response and cuz he's tvr meister. /be
Attachment #256972 -
Flags: review?(brendan) → review-
Assignee | ||
Comment 8•17 years ago
|
||
Brendan: I added JSString to the JSTempValueUnion structure because the macro rooting macro here which was ported from the trunk relies on it, and the macro/type-safe union seems cleaner than using a cast or void *. Hopefully you or Igor will correct me if that is still not correct. I have added the JSString * to the union in the same place as it is added on the trunk.
Attachment #256972 -
Attachment is obsolete: true
Attachment #259144 -
Flags: review?(igor)
Comment 9•17 years ago
|
||
Comment on attachment 259144 [details] [diff] [review] removing unnecessary tvr > /* From here on, control flow must flow through label out. */ > JS_PUSH_SINGLE_TEMP_ROOT(cx, OBJECT_TO_JSVAL(fun->object), &tvr); >+ JS_PUSH_TEMP_ROOT_STRING(cx, atomstr, &tvr); > ok = JS_TRUE; tvr stores the linked list next link so one can not use 2 pushes per one tvr. You need either 2 JSTempValueRooter instances here or better yet one tvr and a jsval[2] array together with code that populates it like in: jsval roots[2]; ... roots[0] = OBJECT_TO_JSVAL(fun->object); roots[1] = STRING_TO_JSVAL(atomstr); JS_PUSH_TEMP_ROOT(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);
Attachment #259144 -
Flags: review?(igor) → review-
Assignee | ||
Comment 10•17 years ago
|
||
Thanks for the explanation, Igor, I started with two tvrs in my original patch; is there a reason one tvr with a jsval array is superior, or is that primarily a style issue?
Attachment #259144 -
Attachment is obsolete: true
Attachment #259156 -
Flags: review?(igor)
Comment 11•17 years ago
|
||
(In reply to comment #10) > Created an attachment (id=259156) [details] > jsval array instead of multiple tvrs > > Thanks for the explanation, Igor, I started with two tvrs in my original patch; > is there a reason one tvr with a jsval array is superior, or is that primarily > a style issue? tvr with jsval[2] just brings less code as the extra linking/unlinking of tvr from the JSContext list is avoided.
Comment 12•17 years ago
|
||
Comment on attachment 259156 [details] [diff] [review] jsval array instead of multiple tvrs > /* From here on, control flow must flow through label out. */ >- JS_PUSH_SINGLE_TEMP_ROOT(cx, OBJECT_TO_JSVAL(fun->object), &tvr); >+ roots[0] = OBJECT_TO_JSVAL(fun->object); >+ roots[1] = STRING_TO_JSVAL(atomstr); >+ JS_PUSH_TEMP_ROOT(cx, sizeof(roots) / sizeof(roots[0]), roots, &tvr); > ok = JS_TRUE; > > if (!JS_XDRStringOrNull(xdr, &atomstr) || Sorry I did not see it the first time, but after decoding atomstr would contain a new string. It is unrooted as roots[1] contains unrelated value. So lets add a second tvr with JSString* member added and then use atomstrTvr.u.string, not atomstr in the rest of code.
Attachment #259156 -
Flags: review?(igor) → review-
Assignee | ||
Comment 13•17 years ago
|
||
I like this patch less (accessing the union elements is ugly), I'd almost rather keep the jsval array and just set the string after the XDR decode. Thoughts? Also, I could of course remove atomstr entirely, but again it looks uglier.
Attachment #259156 -
Attachment is obsolete: true
Comment 14•17 years ago
|
||
Comment on attachment 259226 [details] [diff] [review] two tvrs again, and modified string usage Yes, this is ugly, but at least it works. I also agree that atomizing the string right after JS_XDRStringOrNull and storing it as fun->atom ASAP would also work.
Attachment #259226 -
Flags: review+
Assignee | ||
Comment 15•17 years ago
|
||
(In reply to comment #14) > (From update of attachment 259226 [details] [diff] [review]) > I also agree that atomizing the string right after JS_XDRStringOrNull > and storing it as fun->atom ASAP would also work. Do you mind explaining this part in more depth?
Comment 16•17 years ago
|
||
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 259226 [details] [diff] [review] [details]) > > I also agree that atomizing the string right after JS_XDRStringOrNull > > and storing it as fun->atom ASAP would also work. > > Do you mind explaining this part in more depth? The idea is to move if (xdr->mode == JSXDR_DECODE) { ... if (atomstr) { /* XXX only if this was a top-level function! */ fun->atom = js_AtomizeString(cx, atomstr, 0); if (!fun->atom) goto bad; } } right after JS_XDRStringOrNull(xdr, &atomstr). It this way there is no need to protect atomstr against GC.
Comment 17•17 years ago
|
||
Please avoid atomstrTvr type names -- too many consonants in a row. atomtvr would be nicer, but comment 16 has the better idea. /be
Assignee | ||
Comment 18•17 years ago
|
||
I like this version best so far. And, I'm pretty sure that moving the majority of this chunk up is safe.
Attachment #259421 -
Flags: review?(igor)
Updated•17 years ago
|
Attachment #259421 -
Flags: review?(igor) → review+
Assignee | ||
Comment 19•17 years ago
|
||
The trunk code is way different, so this bug shouldn't be a 1.9 blocker. Gonna try to test this out with tbird now.
Flags: blocking1.9?
Assignee | ||
Comment 20•17 years ago
|
||
Comment on attachment 259421 [details] [diff] [review] atomize the string earlier I believe this bug is either 1.8.0-only, or would present very differently elsewhere.
Attachment #259421 -
Flags: approval1.8.0.12?
Comment 21•17 years ago
|
||
Comment on attachment 259421 [details] [diff] [review] atomize the string earlier approved for 1.8.0.12, a=dveditz for release-drivers
Attachment #259421 -
Flags: approval1.8.0.12? → approval1.8.0.12+
Assignee | ||
Comment 22•17 years ago
|
||
jsfun.c: 3.117.2.7.2.13
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite-
Assignee | ||
Comment 24•17 years ago
|
||
Yes, though in communicating with an external embedder, I'm thinking I may have just moved this or discovered another bug nearby. I will post more when I have had time to work it out with them.
Comment 25•17 years ago
|
||
guessing sg:moderate, but could be sg:critical if there's a reliable web-content testcase that doesn't require particular extensions.
Whiteboard: [sg:moderate?]
Updated•17 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•