1389406 - XSLT — nested xsl:apply-imports doesn't work in some cases

Status: RESOLVED FIXED

(Core :: XSLT, defect, P3)

Description

[Ruvim Pinka]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Open a webpage with application/xml media type, declared text/xsl xml-stylesheet and several nested imports with xsl:apply-imports instructions in XSL files.
The testcase will be added.



Actual results:

Firefox shows error message:
  Error during XSLT transformation: XSLT Stylesheet (possibly) contains a recursion

This issue arose in some Firefox update before June 15 and it is reproducible in version 54.0.1 too. Many web-pages with client side XSLT became inaccessible in Firefox after that.



Expected results:

XML stylesheets should be applied without any error as it was before the update.

Comment 1

[Ruvim Pinka]

Attachment: testcase-01 stylesheet level 03

Comment 2

[Ruvim Pinka]

Attachment: testcase-01 stylesheet level 02

Comment 3

[Ruvim Pinka]

Attachment: testcase-01 stylesheet level 01

Comment 4

[Ruvim Pinka]

Attachment: testcase-01 main xml file

Comment 5

[Ruvim Pinka]

Attachment: testcase-02 stylesheet level 02

Comment 6

[Ruvim Pinka]

Attachment: testcase-02 stylesheet level 01

Comment 7

[Ruvim Pinka]

Attachment: testcase-02 main xml file

Comment 8

[Andrew Overholt [:overholt]]

Eric, can we do something here?

Comment 9

[Ruvim Pinka]

[Tracking Requested - why for this release]: Firefox 57.0a1 Nightly is affected by this bug — it is tested by me now and it doesn't pass the testcase.
Despite the manual (https://wiki.mozilla.org/Bugmasters/Marking_bugs_for_the_Firefox_57_release), the "affected" word is not listed in the Status section of Firefox Tracking Flags section for me.

Comment 10

[Alice0775 White]

Regressed window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d762468d46577d903caf0e0b8cf281b5c9220562&tochange=421407ec02c07437ced56c950b03c74e6faea56b


Regressed by: Bug 1336830, Bug 1336828, Bug 1336832, Bug 1338277

Comment 11

[Eric Rahm [:erahm]]

(In reply to Alice0775 White from comment #10)
> Regressed window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=d762468d46577d903caf0e0b8cf281b5c9220562&tochange=4214
> 07ec02c07437ced56c950b03c74e6faea56b
> 
> 
> Regressed by: Bug 1336830, Bug 1336828, Bug 1336832, Bug 1338277

Looks like patches Peter wrote, I would guess its https://hg.mozilla.org/integration/mozilla-inbound/rev/27bf27ed237f as that's the only one dealing with imports. FWIW the test cases run fine in Chrome.

Peter WDYT?

Comment 12

[Peter Van der Beken [:peterv]]

Yeah, going to have to think this through, need to avoid regressing bug 1336830.

Comment 13

[Julien Cristau [:jcristau] (back April 22)]

We've shipped multiple releases with this, so it doesn't feel worth tracking.

Comment 14

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acebc533ee4f78b71987d1881bb2484187525ad5

Comment 15

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fd5c23317e0472cc39b19291d713b415d1aaab6

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v1

This reverts the part of bug 1336830 that caused the regression (txApplyImportsStart/txApplyImportsEnd instead of txApplyImports). But it avoids the problem that that causes by refcounting parameter maps, since they can be stored in multiple places. I avoided doing the same change for variable maps, I don't think they suffer from the same issue and I want to avoid more regressions :-).

Comment 17

[Eric Rahm [:erahm]]

Comment on attachment 8907656 [details] [diff] [review]
v1

Review of attachment 8907656 [details] [diff] [review]:
-----------------------------------------------------------------

A few questions so f+ for now but overall looks good.

::: dom/xslt/xslt/txExecutionState.cpp
@@ +521,2 @@
>  {
> +    mParamStack.AppendElement(mTemplateParams.forget());

Should we do a fallible append given this is user controlled content?

::: dom/xslt/xslt/txInstructions.cpp
@@ +42,5 @@
> +txApplyImportsEnd::execute(txExecutionState& aEs)
> +{
> +    aEs.popTemplateRule();
> +    RefPtr<txParameterMap> paramMap = aEs.popParamMap();
> +    

nit: trailing space

@@ +72,5 @@
>      aEs.pushTemplateRule(frame, mode, rule->mParams);
>  
>      rv = aEs.runTemplate(templ);
> +    if (NS_FAILED(rv)) {
> +      aEs.popTemplateRule();

Do we need a `popParamMap` as well? Or does `txPopParams` always get run even in the failure case? Same goes for the NS_ENSURE_SUCCESS above. I get the feeling I've already asked this question and it's probably fine.

::: dom/xslt/xslt/txVariableMap.h
@@ +38,3 @@
>  
> +private:
> +    txExpandedNameMap<txAExprResult> mMap;

This shadowing is a bit...odd. Should we just make `txVariableMapBase::mMap` protected? I think it's probably making `~txVariableMap` a no-op. Or if I'm wrong maybe add a comment about what's going on here at least.

Actually either way can you add comments on the difference? I guess `txVariableMap` is owning but non-ref counted and `txParamMap` is non-owning but ref counted?

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: v1.1

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #17)
> ::: dom/xslt/xslt/txExecutionState.cpp
> @@ +521,2 @@
> >  {
> > +    mParamStack.AppendElement(mTemplateParams.forget());
> 
> Should we do a fallible append given this is user controlled content?

I think this is fine because we do a recursion check in txExecutionState::runTemplate (called by txApplyImportsStart::execute).

> @@ +72,5 @@
> >      aEs.pushTemplateRule(frame, mode, rule->mParams);
> >  
> >      rv = aEs.runTemplate(templ);
> > +    if (NS_FAILED(rv)) {
> > +      aEs.popTemplateRule();
> 
> Do we need a `popParamMap` as well? Or does `txPopParams` always get run
> even in the failure case? Same goes for the NS_ENSURE_SUCCESS above. I get
> the feeling I've already asked this question and it's probably fine.

Now that the param map is reference counted it will just get cleaned up when the ExecutionState destructor causes the mParamStack
nsTArray destructor to run. We used to have to deal with cleanup because there are multiple owners, but with reference counting that's fine, all owners just release their ref whenever.

> ::: dom/xslt/xslt/txVariableMap.h
> @@ +38,3 @@
> >  
> > +private:
> > +    txExpandedNameMap<txAExprResult> mMap;
> 
> This shadowing is a bit...odd. Should we just make `txVariableMapBase::mMap`
> protected? I think it's probably making `~txVariableMap` a no-op. Or if I'm
> wrong maybe add a comment about what's going on here at least.
> 
> Actually either way can you add comments on the difference? I guess
> `txVariableMap` is owning but non-ref counted and `txParamMap` is non-owning
> but ref counted?

Sigh, this was a mess, sorry. I cleaned it up and added comments.

Comment 19

[Eric Rahm [:erahm]]

Comment on attachment 8909678 [details] [diff] [review]
v1.1

Review of attachment 8909678 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good!

Comment 20

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba719124ab62b79ca77510c0a979604830f9c5e
Bug 1389406 - XSLT — nested xsl:apply-imports doesn't work in some cases. r=erahm.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ba719124ab6

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Peter, did you want to let this ride the 58 train or were you thinking about requesting Beta approval on it?

Comment 23

[Sylvestre Ledru [:Sylvestre]]

We already shipped several releases with it. I guess we can let it ride the trains.