code review 12420044: text/template/parse, html/template: copy Tree.Text duri...

text/template/parse, html/template: copy Tree.Text during html template clone

The root cause of the panic reported in https://code.google.com/p/go/issues/detail?id=5980
is that parse's Tree.Text wasn't being copied during the clone.

Note that this fix newly exports Tree.Text. The only alternative fix that does
not involve API additions is to significantly degrade error messages.

Fixes issue 5980.

[josharian, 2013-08-04 15:22:47]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[r, 2013-08-05 00:50:29]

This CL changes the semantics of html/template's Clone method. See the comment
at the top, which says it does not do what you're now making it do.

More analysis is required. For one thing, can this error be triggered in
text/template? If not, why not?

[josharian, 2013-08-06 01:34:25]

On 2013/08/05 00:50:29, r wrote:
> This CL changes the semantics of html/template's Clone method. See the comment
> at the top, which says it does not do what you're now making it do.

I assume you mean the comment "...the name space of associated templates is, so
further calls to Parse in the copy will add templates to the copy but not to the
original."

I'm afraid I don't follow why; my apologies. I added a (simplistic) test for
that in Patch Set 4 -- see TestCloneThenParse in clone_test.go. It passes. What
am I missing?


> More analysis is required. For one thing, can this error be triggered in
> text/template? If not, why not?

Yes: See TestErrorContextWithShallowTreeCopy in parse_text.go, added in Patch
Set 5. This panic can be easily triggered from anywhere using the same
technique. html/template is just the only place in the stdlib where it occurs.

Maybe I still misunderstand, but if not, here are the alternatives I see:

  (0) Add parse.Tree.Copy(). Document that it is the only safe way to make
copies (missing from this CL). Better, but alas not an option now, convert Tree
into an interface.
  (1) Export the parse.Tree text field, or (ick) provide getters and setters.
  (2) Seriously degrade ErrorContext when tree is nil.

I'd love to be wrong, though. :)

Thanks for your help with this, much appreciated.

-josh

[josharian, 2013-09-13 15:45:47]

Hello golang-dev@googlegroups.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[josharian, 2013-09-13 15:47:59]

On 2013/09/13 15:45:47, josharian wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:r@golang.org (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

...and if you'd prefer the "degrade error messages" approach to exporting
Tree.Text, no problem, happy to do that instead.

-josh

[r, 2013-09-17 03:44:26]

here's what i think:

if you add this method to text/template/parse:

// Copy returns a copy of the Tree. Any parsing state is discarded.
func (t *Tree) Copy() *Tree {
  if t == nil {
  return nil
}
  return &Tree{
    Name:      t.Name,
    ParseName: t.ParseName,
    Root:      t.Root.CopyList(),
    text:      t.text,
  }
}

then do this diff:

--- a/src/pkg/html/template/template.go  Mon Sep 16 20:31:21 2013 -0400
+++ b/src/pkg/html/template/template.go  Tue Sep 17 13:42:55 2013 +1000
@@ -190,12 +190,7 @@
     if src == nil || src.escaped {
       return nil, fmt.Errorf("html/template: cannot Clone %q after it
has executed", t.Name())
     }
-    if x.Tree != nil {
-      x.Tree = &parse.Tree{
-        Name: x.Tree.Name,
-        Root: x.Tree.Root.CopyList(),
-      }
-    }
+    x.Tree = x.Tree.Copy()
     ret.set[name] = &Template{
       false,
       x,

the solution is much more satisfying to me. i like your tests, though.

i can put in these changes and then you can just send the tests, or
you can take these changes and incorporate them into your CL. which do
you prefer?

-rob

[josharian, 2013-09-17 04:08:12]

> here's what i think:
>
> if you add this method to text/template/parse:
>
> // Copy returns a copy of the Tree. Any parsing state is discarded.
> func (t *Tree) Copy() *Tree {
>   if t == nil {
>   return nil
> }
>   return &Tree{
>     Name:      t.Name,
>     ParseName: t.ParseName,
>     Root:      t.Root.CopyList(),
>     text:      t.text,
>   }
> }
>
> then do this diff:
>
> --- a/src/pkg/html/template/template.go  Mon Sep 16 20:31:21 2013 -0400
> +++ b/src/pkg/html/template/template.go  Tue Sep 17 13:42:55 2013 +1000
> @@ -190,12 +190,7 @@
>      if src == nil || src.escaped {
>        return nil, fmt.Errorf("html/template: cannot Clone %q after it
> has executed", t.Name())
>      }
> -    if x.Tree != nil {
> -      x.Tree = &parse.Tree{
> -        Name: x.Tree.Name,
> -        Root: x.Tree.Root.CopyList(),
> -      }
> -    }
> +    x.Tree = x.Tree.Copy()
>      ret.set[name] = &Template{
>        false,
>        x,
>
> the solution is much more satisfying to me. i like your tests, though.
>
> i can put in these changes and then you can just send the tests, or
> you can take these changes and incorporate them into your CL. which do
> you prefer?


LGTM.

It's nigh on bedtime here. In the interests of keeping things moving along,
why don't you just borrow my tests and submit it as one CL. (I feel no
particular need to have my name attached to commits; just want to chip in.)

Josh

[r, 2013-09-17 04:14:47]

LGTM

[r, 2013-09-17 04:19:53]

*** Submitted as https://code.google.com/p/go/source/detail?r=285560f2ebe8 ***

text/template/parse, html/template: copy Tree.text during html template clone

The root cause of the panic reported in
https://code.google.com/p/go/issues/detail?id=5980
is that parse's Tree.Text wasn't being copied during the clone.

Fix this by adding and using a Copy method for parse.Tree.

Fixes issue 5980.

R=golang-dev, r
CC=golang-dev
https://codereview.appspot.com/12420044

Committer: Rob Pike <r@golang.org>