485286 - XSLT should heap allocate all evalContexts

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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).

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: testcase

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This is ready to be checked in.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 369449 [details] [diff] [review]
Fix

Ok, that makes sense. I've also looked for
other cases and couldn't see anything wrong.

Comment 5

[Samuel Sidler (old account; do not CC)]

Comment on attachment 369449 [details] [diff] [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.

Comment 6

[Samuel Sidler (old account; do not CC)]

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.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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)

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For the 1.8 branch

Peter, mind sanity checking the backport? Thanks. Hopefully, this will be the last we hear of this bug.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Note: sicking's testcase relies on bug 353364, so will need tweaking to test the 1.9.0 and 1.8 branches.

Comment 10

[Daniel Veditz [:dveditz]]

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

Comment 11

[Alexander Sack]

i can confirm that the 1.8 branch patch makes the testcase not-crash for me on 1.8.1. Thanks!

Comment 12

[juan becerra [:juanb]]

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 13

[Samuel Sidler (old account; do not CC)]

Comment on attachment 369457 [details] [diff] [review]
For the 1.8 branch

Approved for 1.8.1.22. a=ss for release-drivers.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

/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 15

[Alexander Sack]

Comment on attachment 369457 [details] [diff] [review]
For the 1.8 branch

a=asac for 1.8.0

verified that this fixes the testcase on 1.8.0 too

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 17

[Samuel Sidler (old account; do not CC)]

(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?

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Crashtest

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.8.1.22 using testcase in comment #10.