91332 - Crash while applying templates to key() results

Status: VERIFIED FIXED

(Core :: XSLT, defect, P3)

Description

[xyzzy]

When the stylesheet is loaded, the browser crashes on my system.  I'm using 
Windows 95 with build 2001071604

Think this is legit syntax, but it shouldn't crash in any case...

Reproducible: Always
Steps to Reproduce:
1. File -> Open File ...
2. Choose crash.xml

Actual Results:  Crash

Expected Results:  A table should be generated with a static message for each 
entry in the table.  With the offending template removed, default templates work 
correctly in a similar fashion.

Comment 1

[xyzzy]

Attachment: testcase (XML)

Comment 2

[xyzzy]

Attachment: testcase (XSL)

Comment 3

[xyzzy]

Is this caused by the implementation of bug 65986?

Comment 4

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

Yep, this is all mine :((

Comment 5

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

This one is really really strange. The crash is in txXSLKey::testNode on line 
219. The vtable pointer for key->matchPattern seems totally screwed up. Either 
there is some wrong casting somewhere in here, or we overwrite that memory 
somewhere.

Could someone test if this occurs on other systems then windows?

Comment 6

[Axel Hecht]

Hey, I found out what's happening.

The Pattern "quad" is used for both the template and the key.

First, the key is generated, and quad is parsed. Then the template is parsed,
quad gets returned from the cache, and whooops, ownership destroyed.
Looks like this is the offending code:
ProcessorState.cpp:197
    if (match.length() > 0) {
        patternExprHash.put(match, exprParser.createPatternExpr(match));
        templates.add(xslTemplate);
    }
The put is brute force, does this hork when we use the same pattern for two
templates, too? (Remember xsl:import)

Oh, and we probably should safe-guard NamedMap.cpp:256 for the 
bktItem->item==obj case.

Axel

Comment 7

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

Ahhh, cool. Excellent detective work, you da man. So does it work if we just 
remove that bruteforce (I think it should)?

I agree on the safe-guard, but it wouldn't fix this case, since we parse two 
identical, but separate, xpath-objecttrees.

Comment 8

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

This is actually only a problem with keys since they are the only onces to keep 
pointers to expressions returned from ProcessorState::get(Pattern)Expr

There are two possible fixes to this:

1. Change the ownership model for the returned expressions from "don't hold a
   pointer to the returned expression since ProcessorState is allowed to change
   it at any time" to something like "the retuned expression is guaranteed to
   live as long as the current transformation".
   All we would have to do is to remove that bruteforce

2. Let keys hold a pointer to the element and get the match-pattern and
   use-expression from ProcessorState::get(Pattern)Expr whenever needed.
   Whenever needed is once per source document.

I'd say that we defenetly should do 1, but I don't see a reason to not do 2 
aswell actually (insigificant speedloss and small memory and codesize win)

Thoughts?

Comment 9

[Axel Hecht]

... and there is the right way to fix it.

ProcessorState::templates shouldn't be a NodeList, but a List of
struct txTemplate
{
    Node* mXSLTemplate;
    Expr* mMatchPattern;
}
the ExprParser should be held in XSLTProcessor, and the ProcessorState just
holds the list.
Get rid of that "get attribute, look in hash" piece of ugliness.

And then keys just run in the family. They have their own patterns, expressions
or whatever, and can delete them whenever they feel like it.

Axel

Comment 10

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

While I agree that we need that struct it's IMHO not part of this bug. Is it ok 
if I just remove the bruteforce and then do that struct some other time (while 
implementing importprecedence for example)

Comment 11

[Peter Van der Beken [:peterv]]

I'd prefer the struct too. Either you do a workaround for 0.9.4, and leave the
bug open for the struct, or do the struct now. Oh, and did I mention I prefer
the struct too?

Comment 12

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

Yes you mentioned that :), and I like the struct too. There are a couple of 
nice things we need that struct for (sort templates by priority and LRE 
stylesheets) but I don't see how the struct is related to this bug?

Just to clairify: IMHO the expressions/patterns should still be owned by the 
hashes in ProcessorState, since we need those hashes anyway (for stuff like 
xsl:apply-templates and xsl:value-of) so I don't see any harm in using them for 
the expressions for keys and templates. However I want to change the guaranteed 
lifetime from "not long" to "as long as the transformation is running"

Comment 13

[Axel Hecht]

The fundamental difference is, Expr and Patterns shouldn't be in NamedMaps.
It's just wrong to cache them on string value, as this might resolve to 
different namespaces, and foo like that.

I am not sure which expressions/patterns to hash, and which we should just
generate on demand and dump (xsl:number seems a candidate for the latter).

Stuff that needs care:
[Expr]
apply-templates/@select
copy-of/@select
for-each/@select
if/@test
key/@use
number/@value
sort/@select
value-of/@select
variable/@select
when/@test
with-param/@select
[Pattern]
key/@match
number/(@count|@from)

Axel

Comment 14

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

Oh, yeah, sorry. We shouldn't key them on string. Forgot to say that.

IMHO we should key them on what node in the xslt tree they are contained in 
(which is always an attribute node AFAICT). That will get rid of all namespace 
and baseURI and foo problems.

I don't think we should parse and drop any expressions since parsing (and 
optimising) isn't that cheap.

I guess we could skip sticking some of the parsed expressions into the hashes, 
e.g. for templates and keys, and let somebody else own the expressions. The 
advantage of that is that it would keep the hash smaller and therefor make 
lookups in it faster (with our current sucky hash), the disadvantage is that it 
complicates the cleanup code a bit.

Comment 15

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

Attachment: fixes the crash

Comment 16

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

This fixes the crash. While I 100% agree that we should do that other stuff 
it's IMHO not part of this bug.

Comment 17

[Axel Hecht]

ok, let's move the right hashing to another bug.

But don't just remove the Pattern creation. Peter and I chatted on this, and 
the right thing is to call ProcessorState::getPatternExpr, like everybody
else.

Axel

Comment 18

[Axel Hecht]

filed bug 95779 on the caching issue

Comment 19

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

ProcessorState::getPatternExpr is called in ProcessorState::findTemplate, so I 
don't really understand why it was bruteforced into the hash at all? The only 
thing that'll accomplish is that the template-patterns will be added in the 
hash a little sooner, but they'll all go in there on the first call 
to ::findTemplate anyway.

I don't really mind adding a dummycall there to make sure the pattern goes into 
the hash, I just don't see the point.

Your call...

Comment 20

[Axel Hecht]

The difference is, once we implement 95779, this is the place to put the match 
pattern into the template struct, so let's keep the code where it needs to
be.
That's why we called for getPatternExpr.

Axel

Comment 21

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

Attachment: addresses Pikes and petervs dope requests

Comment 22

[Axel Hecht]

We, that is the GODMOMMA OF BUGSHIT and I, review 46233.
Peterv is still on carpet.

Axel

Comment 23

[Mike Shaver (:shaver emeritus)]

sr=shaver

Comment 24

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

checked in

Comment 25

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

verified

Comment 26

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/5a6def05ccbc