txExprParser::createExpr should either return NS_FAILED or an Expr

RESOLVED FIXED

Status

()

Core
XSLT
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Axel Hecht, Assigned: Axel Hecht)

Tracking

({crash, regression})

Trunk
crash, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

15 years ago
the nsresult returned by createExpr should be either a error value or a 
non-null expr, no mixture.
This means that fcp needs to be handled outside of createExpr in 
txStylesheetCompileHandlers.cpp, getExprAttr.
Any progress on this? This is a pretty serious regression since crashers were
introduced.
Keywords: crash, regression
err.. pressed submit prematurly. 1.6a is comming up in a couple of days and it'd
suck to have that released with known xslt regressions.
(Assignee)

Comment 3

15 years ago
Created attachment 133755 [details] [diff] [review]
fixing the leak
(Assignee)

Comment 4

15 years ago
I don't see a crash anywhere. We never return NS_OK for a null expression. 
This code leaked, though.

Note, I'm not doing fcp. I don't expect to get reviews on anything I do, so I'm
not doing anything.
Rationale for my opinion:
fcp is pretty useless in its own right, xslt1.1 is dead and xslt2 is just too
far away to be actually of any use in a 1.0 processor.
I'd only do a single expression, hooked up in getExprAttr to get fcp done, which
conflicts with the language of the fcp spec as far as function argument counts 
go.
That says "should only error if the function is called". Of course the only way
for that not to happen in a expr evaluation would be a 
(function-available(foo) and foo('way', 'too', 'many', 'arguments')).
Of course that's pretty useless in most cases, as that works only for boolean
function call results *and* under the assumption that false is a good default
value. It's just silly language stating something completely irrelevant.
Not gonna go on my time.
Status: NEW → ASSIGNED
Keywords: crash
(Assignee)

Updated

15 years ago
Attachment #133755 - Flags: superreview?(peterv)
Attachment #133755 - Flags: review?(bugmail)
Attachment #133755 - Flags: superreview?(peterv) → superreview+
it returns NS_OK and a null-expression for the empty string so the current patch
only fixes part of this bug.
Keywords: crash
Comment on attachment 133755 [details] [diff] [review]
fixing the leak

upps, that should be |*aExpr| and not |aExpr|
Attachment #133755 - Flags: review+ → review-
Created attachment 133780 [details] [diff] [review]
patch to fix

The change to the lexer makes the lexer treat "" as " ", which then the parser
catches as a parse-error
Attachment #133780 - Flags: superreview+
Attachment #133780 - Flags: review+
checked in, thanks for reviews
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.