Bugzilla@Mozilla – Bug 485286
XSLT should heap allocate all evalContexts
Last modified: 2009-06-04 14:32:58 PDT
Summon comment box
Created attachment 369413 [details] [review] Proposed fix bug 485217 fixed one case where we fail to pop a stack-allocated evalContext in an error condition. However, with the patch in that bug, it's possible to re-enter the interpreter and push a heap-allocated eval context (via a txPushNewContext instruction). If, after we handle such an instruction, we hit an error, then we'll leave that eval context on the stack and, when we retreat back into the code fixed in bug 485217, we'll pop the *wrong* context, leaving the same error as before. The patch I'm attaching basically backs out the patch in bug 485217 per sicking's request to make the error handling consistent (you never pop someone else's context, but all contexts are heap allocated, so we can safely clean them up at the end).
Created attachment 369416 [details] testcase
Forgot to make one comment. We should check for out-of-memory when allocating the context. Whoever checks this in it'd be great to fix that, but it's not really very critical. I also look through the code and couldn't find any other cases where this pattern occurs.
Created attachment 369449 [details] [review] Fix This is ready to be checked in.
Comment on attachment 369449 [details] [review] Fix Ok, that makes sense. I've also looked for other cases and couldn't see anything wrong.
Comment on attachment 369449 [details] [review] Fix Approved for 1.9.0.8 and 1.9.0.9. a=ss Please land on the 1.9.0 branch ASAP. We also need to get this on the relbranch for 1.9.0.8.
Blocking all around. I can't set 1.9.1 blocking, but it blocks that too. Blake will make sure this applies to 1.8 (or attach a 1.8 patch) tomorrow. 1.8.1/1.8.0 Linux distro drivers: We're taking this patch with our firedrill. It's a more complete fix for bug 485217 and we don't want to re-0-day ourselves.
http://hg.mozilla.org/mozilla-central/rev/85bd18f6b652 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4552d789ad8f Also: content/xslt/src/xslt/txKeyFunctionCall.cpp: 1.42 -> 1.43 (1.9.0) and 1.41.30.1 -> 1.41.30.2 (relbranch)
Created attachment 369457 [details] [review] For the 1.8 branch Peter, mind sanity checking the backport? Thanks. Hopefully, this will be the last we hear of this bug.
Note: sicking's testcase relies on bug 353364, so will need tweaking to test the 1.9.0 and 1.8 branches.
fwiw I tested that Blake's initial patch fixed the testcase in bug 485217 and did not fix the testcase here, and that this patch fixes both testcases. Tested both 1.9.1 and 1.9.0 builds (the tweaking in comment 9 is replacing href="" with an explicit full URI to wherever you've put that file). For now Sam has put an appropriately tweaked copy at http://people.mozilla.com/~ss/485286-shh/testcase.xml
i can confirm that the 1.8 branch patch makes the testcase not-crash for me on 1.8.1. Thanks!
Verified fixed on the following builds using the testcase in comment #10: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729) Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.8) Gecko/2009032608 Firefox/3.0.8 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8) Gecko/2009032608 Firefox/3.0.8 Fx307 on Mac, Linux and Windows crashed. Fx308 does not crash on this testcase.
Comment on attachment 369457 [details] [review] For the 1.8 branch Approved for 1.8.1.22. a=ss for release-drivers.
/cvsroot/mozilla/extensions/transformiix/source/xslt/functions/Attic/txKeyFunctionCall.cpp,v <-- txKeyFunctionCall.cpp new revision: 1.33.28.2; previous revision: 1.33.28.1
Comment on attachment 369457 [details] [review] For the 1.8 branch a=asac for 1.8.0 verified that this fixes the testcase on 1.8.0 too
Verified on trunk and 1.9.1 with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090329 Minefield/3.6a1pre ID:20090329031037 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090329 Shiretoko/3.5b4pre ID:20090329030931 Would be nice to have this testcase as an automated test.
(In reply to comment #16) > Would be nice to have this testcase as an automated test. Blake, can you get an automated test for this landed in the various places?
Created attachment 370042 [details] [review] Crashtest
http://hg.mozilla.org/mozilla-central/rev/f02ddd383a4b and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cb01d655a1b1 and the 1.9.0 branch.
Verified for 1.9.0.9 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.9pre) Gecko/2009040204 GranParadiso/3.0.9pre) with testcase in comment 10. Blake's crashtest also passes on tinderbox.
Verified for 1.8.1.22 using testcase in comment #10.