html/template: revert "avoid race when escaping updates template"

This reverts CLs 274450 and 279492, except for the new tests.
The new race test is changed to skip, as it now fails.
We can try again for 1.17.

Original CL descriptions:

    html/template: attach functions to namespace

    The text/template functions are stored in a data structure shared by
    all related templates, so do the same with the original, unwrapped,
    functions on the html/template side.

    html/template: avoid race when escaping updates template

For #39807
Fixes #43855

Change-Id: I2ce91321ada06ea496a982aefe170eb5af9ba847
Reviewed-on: https://go-review.googlesource.com/c/go/+/285957
Trust: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Emmanuel Odeke <[email protected]>

Author: ianlancetaylor

src/html/template/exec_test.go

@@ -1720,6 +1720,8 @@ var v = "v";
 `
 
 func TestEscapeRace(t *testing.T) {
+	t.Skip("this test currently fails with -race; see issue #39807")
+
 	tmpl := New("")
 	_, err := tmpl.New("templ.html").Parse(raceText)
 	if err != nil {
@@ -1777,6 +1779,39 @@ func TestRecursiveExecute(t *testing.T) {
 	}
 }
 
+// recursiveInvoker is for TestRecursiveExecuteViaMethod.
+type recursiveInvoker struct {
+	t    *testing.T
+	tmpl *Template
+}
+
+func (r *recursiveInvoker) Recur() (string, error) {
+	var sb strings.Builder
+	if err := r.tmpl.ExecuteTemplate(&sb, "subroutine", nil); err != nil {
+		r.t.Fatal(err)
+	}
+	return sb.String(), nil
+}
+
+func TestRecursiveExecuteViaMethod(t *testing.T) {
+	tmpl := New("")
+	top, err := tmpl.New("x.html").Parse(`{{.Recur}}`)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = tmpl.New("subroutine").Parse(`<a href="/x?p={{"'a<b'"}}">`)
+	if err != nil {
+		t.Fatal(err)
+	}
+	r := &recursiveInvoker{
+		t:    t,
+		tmpl: tmpl,
+	}
+	if err := top.Execute(io.Discard, r); err != nil {
+		t.Fatal(err)
+	}
+}
+
 // Issue 43295.
 func TestTemplateFuncsAfterClone(t *testing.T) {
 	s := `{{ f . }}`

src/html/template/template.go

@@ -11,7 +11,6 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"reflect"
 	"sync"
 	"text/template"
 	"text/template/parse"
@@ -36,20 +35,18 @@ var escapeOK = fmt.Errorf("template escaped correctly")
 
 // nameSpace is the data structure shared by all templates in an association.
 type nameSpace struct {
-	mu      sync.RWMutex
+	mu      sync.Mutex
 	set     map[string]*Template
 	escaped bool
 	esc     escaper
-	// The original functions, before wrapping.
-	funcMap FuncMap
 }
 
 // Templates returns a slice of the templates associated with t, including t
 // itself.
 func (t *Template) Templates() []*Template {
 	ns := t.nameSpace
-	ns.mu.RLock()
-	defer ns.mu.RUnlock()
+	ns.mu.Lock()
+	defer ns.mu.Unlock()
 	// Return a slice so we don't expose the map.
 	m := make([]*Template, 0, len(ns.set))
 	for _, v := range ns.set {
@@ -87,8 +84,8 @@ func (t *Template) checkCanParse() error {
 	if t == nil {
 		return nil
 	}
-	t.nameSpace.mu.RLock()
-	defer t.nameSpace.mu.RUnlock()
+	t.nameSpace.mu.Lock()
+	defer t.nameSpace.mu.Unlock()
 	if t.nameSpace.escaped {
 		return fmt.Errorf("html/template: cannot Parse after Execute")
 	}
@@ -97,16 +94,6 @@ func (t *Template) checkCanParse() error {
 
 // escape escapes all associated templates.
 func (t *Template) escape() error {
-	t.nameSpace.mu.RLock()
-	escapeErr := t.escapeErr
-	t.nameSpace.mu.RUnlock()
-	if escapeErr != nil {
-		if escapeErr == escapeOK {
-			return nil
-		}
-		return escapeErr
-	}
-
 	t.nameSpace.mu.Lock()
 	defer t.nameSpace.mu.Unlock()
 	t.nameSpace.escaped = true
@@ -134,8 +121,6 @@ func (t *Template) Execute(wr io.Writer, data interface{}) error {
 	if err := t.escape(); err != nil {
 		return err
 	}
-	t.nameSpace.mu.RLock()
-	defer t.nameSpace.mu.RUnlock()
 	return t.text.Execute(wr, data)
 }
 
@@ -151,36 +136,20 @@ func (t *Template) ExecuteTemplate(wr io.Writer, name string, data interface{})
 	if err != nil {
 		return err
 	}
-	t.nameSpace.mu.RLock()
-	defer t.nameSpace.mu.RUnlock()
 	return tmpl.text.Execute(wr, data)
 }
 
 // lookupAndEscapeTemplate guarantees that the template with the given name
 // is escaped, or returns an error if it cannot be. It returns the named
 // template.
 func (t *Template) lookupAndEscapeTemplate(name string) (tmpl *Template, err error) {
-	t.nameSpace.mu.RLock()
+	t.nameSpace.mu.Lock()
+	defer t.nameSpace.mu.Unlock()
+	t.nameSpace.escaped = true
 	tmpl = t.set[name]
-	var escapeErr error
-	if tmpl != nil {
-		escapeErr = tmpl.escapeErr
-	}
-	t.nameSpace.mu.RUnlock()
-
 	if tmpl == nil {
 		return nil, fmt.Errorf("html/template: %q is undefined", name)
 	}
-	if escapeErr != nil {
-		if escapeErr != escapeOK {
-			return nil, escapeErr
-		}
-		return tmpl, nil
-	}
-
-	t.nameSpace.mu.Lock()
-	defer t.nameSpace.mu.Unlock()
-	t.nameSpace.escaped = true
 	if tmpl.escapeErr != nil && tmpl.escapeErr != escapeOK {
 		return nil, tmpl.escapeErr
 	}
@@ -286,13 +255,6 @@ func (t *Template) Clone() (*Template, error) {
 	}
 	ns := &nameSpace{set: make(map[string]*Template)}
 	ns.esc = makeEscaper(ns)
-	if t.nameSpace.funcMap != nil {
-		ns.funcMap = make(FuncMap, len(t.nameSpace.funcMap))
-		for name, fn := range t.nameSpace.funcMap {
-			ns.funcMap[name] = fn
-		}
-	}
-	wrapFuncs(ns, textClone, ns.funcMap)
 	ret := &Template{
 		nil,
 		textClone,
@@ -307,13 +269,12 @@ func (t *Template) Clone() (*Template, error) {
 			return nil, fmt.Errorf("html/template: cannot Clone %q after it has executed", t.Name())
 		}
 		x.Tree = x.Tree.Copy()
-		tc := &Template{
+		ret.set[name] = &Template{
 			nil,
 			x,
 			x.Tree,
 			ret.nameSpace,
 		}
-		ret.set[name] = tc
 	}
 	// Return the template associated with the name of this template.
 	return ret.set[ret.Name()], nil
@@ -382,43 +343,10 @@ type FuncMap map[string]interface{}
 // type. However, it is legal to overwrite elements of the map. The return
 // value is the template, so calls can be chained.
 func (t *Template) Funcs(funcMap FuncMap) *Template {
-	t.nameSpace.mu.Lock()
-	if t.nameSpace.funcMap == nil {
-		t.nameSpace.funcMap = make(FuncMap, len(funcMap))
-	}
-	for name, fn := range funcMap {
-		t.nameSpace.funcMap[name] = fn
-	}
-	t.nameSpace.mu.Unlock()
-
-	wrapFuncs(t.nameSpace, t.text, funcMap)
+	t.text.Funcs(template.FuncMap(funcMap))
 	return t
 }
 
-// wrapFuncs records the functions with text/template. We wrap them to
-// unlock the nameSpace. See TestRecursiveExecute for a test case.
-func wrapFuncs(ns *nameSpace, textTemplate *template.Template, funcMap FuncMap) {
-	if len(funcMap) == 0 {
-		return
-	}
-	tfuncs := make(template.FuncMap, len(funcMap))
-	for name, fn := range funcMap {
-		fnv := reflect.ValueOf(fn)
-		wrapper := func(args []reflect.Value) []reflect.Value {
-			ns.mu.RUnlock()
-			defer ns.mu.RLock()
-			if fnv.Type().IsVariadic() {
-				return fnv.CallSlice(args)
-			} else {
-				return fnv.Call(args)
-			}
-		}
-		wrapped := reflect.MakeFunc(fnv.Type(), wrapper)
-		tfuncs[name] = wrapped.Interface()
-	}
-	textTemplate.Funcs(tfuncs)
-}
-
 // Delims sets the action delimiters to the specified strings, to be used in
 // subsequent calls to Parse, ParseFiles, or ParseGlob. Nested template
 // definitions will inherit the settings. An empty delimiter stands for the
@@ -432,8 +360,8 @@ func (t *Template) Delims(left, right string) *Template {
 // Lookup returns the template with the given name that is associated with t,
 // or nil if there is no such template.
 func (t *Template) Lookup(name string) *Template {
-	t.nameSpace.mu.RLock()
-	defer t.nameSpace.mu.RUnlock()
+	t.nameSpace.mu.Lock()
+	defer t.nameSpace.mu.Unlock()
 	return t.set[name]
 }