Closed
Bug 369714
Opened 18 years ago
Closed 18 years ago
Thunderbird crash on startup [@js_HashString()]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jwkbugzilla, 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•18 years ago
|
||
Reporter | ||
Updated•18 years ago
|
Attachment #254402 -
Attachment mime type: text/plain → application/octet-stream
Updated•18 years ago
|
Flags: blocking1.9?
Reporter | ||
Comment 2•18 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•18 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•18 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•18 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•18 years ago
|
Status: NEW → ASSIGNED
Comment 7•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 years ago
|
Attachment #259421 -
Flags: review?(igor) → review+
Assignee | ||
Comment 19•18 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•18 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•18 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•18 years ago
|
||
jsfun.c: 3.117.2.7.2.13
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: in-testsuite-
Assignee | ||
Comment 24•18 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•18 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•18 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•