Bug 104142 – [HTMLTemplateElement] Prevent first-level recursive <template> from resetting the implied context

• Component: DOM
• Status: RESOLVED FIXED

[Rafael Weinstein]

Hey guys,

Here's my take on resolving: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20130

Is this a reasonable approach? If so, I'll update the spec, refer to it in this patch, then request formal review.

[Rafael Weinstein]

Created attachment 177796 [details]
Patch

[Adam Barth]

Comment on attachment 177796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177796&action=review

> Source/WebCore/ChangeLog:8
> +        This patch adds a "stack" (although the impl is just a map) which allows for retaining the chosen "implied context" for each template element.

I see, it is just a stack.  Is there a reason you used a HashMap rather than a Vector to implement the stack?  A Vector seems both more efficient and to be a clearer representation of a stack.

> Source/WebCore/html/parser/HTMLTreeBuilder.h:237
> +#if ENABLE(TEMPLATE_ELEMENT)
> +    HashMap<Element*, InsertionMode> m_templateInsertionModes;
> +#endif

Interesting!  Does this need to be a hashmap?  I would have expected just a Vector<InsertionMode> that we'd push and pop in parallel with the stack of open elements.  (We can overwrite the top of the stack in processStartTag.

[Adam Barth]

Yeah, this looks like a good way to resolve this issue (modulo implementation details).

[Adam Klein]

Created attachment 178095 [details]
Patch

[Adam Klein]

(In reply to comment #2)
> (From update of attachment 177796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177796&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        This patch adds a "stack" (although the impl is just a map) which allows for retaining the chosen "implied context" for each template element.
> 
> I see, it is just a stack.  Is there a reason you used a HashMap rather than a Vector to implement the stack?  A Vector seems both more efficient and to be a clearer representation of a stack.
> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.h:237
> > +#if ENABLE(TEMPLATE_ELEMENT)
> > +    HashMap<Element*, InsertionMode> m_templateInsertionModes;
> > +#endif
> 
> Interesting!  Does this need to be a hashmap?  I would have expected just a Vector<InsertionMode> that we'd push and pop in parallel with the stack of open elements.  (We can overwrite the top of the stack in processStartTag.

Switched to a vector-as-stack.

[Adam Barth]

Comment on attachment 178095 [details]
Patch

Should we assert that the stack is empty when we're done parsing?

[Adam Klein]

(In reply to comment #6)
> (From update of attachment 178095 [details])
> Should we assert that the stack is empty when we're done parsing?

Well, right now, it's not always empty. For example, in the fragment-parsing mode, it's always size 1 when we finish. And I haven't added code to, e.g., processEndOfFile to clear out the stack. But I am a little worried about it getting out of sync with the stack of open elements. If I were going to add such an assert, would it go in finished()?

[Rafael Weinstein]

Thanks for taking over this one as well.

FWIW, I'm fine with what you guys decide here. The reason why I used a hashmap rather than a vector is that I was worried about maintaining a 'stack within a stack'. In other words, its seems to me that that the only logical stack here is the stack of open elements and that the map was encoding a property which gets applied to each <template> encountered during parsing, but that properties lifetime is tied to a single invocation of the parser.

[Adam Klein]

(In reply to comment #8)
> Thanks for taking over this one as well.
> 
> FWIW, I'm fine with what you guys decide here. The reason why I used a hashmap rather than a vector is that I was worried about maintaining a 'stack within a stack'. In other words, its seems to me that that the only logical stack here is the stack of open elements and that the map was encoding a property which gets applied to each <template> encountered during parsing, but that properties lifetime is tied to a single invocation of the parser.

For what it's worth, running the original (HashMap-based) patch in debug mode caused run-template.html to crash for me. I think there may be some issues with having to get ahold of the current template when wanting to find the current template insertion mode.

Perhaps we should integrate this enum stack directly into the element stack?

[Adam Klein]

Created attachment 179530 [details]
Patch with better ASSERTs

[Eric Seidel (no email)]

Comment on attachment 179530 [details]
Patch with better ASSERTs

Seems reasonable.

[Eric Seidel (no email)]

The review tool seems to have lost all my comments!?!

[Eric Seidel (no email)]

Comment on attachment 179530 [details]
Patch with better ASSERTs

View in context: https://bugs.webkit.org/attachment.cgi?id=179530&action=review

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2517
> +#if ENABLE(TEMPLATE_ELEMENT) && !ASSERT_DISABLED

Why is this disabled in release?

> LayoutTests/html5lib/resources/template.dat:847
> +<body><template><thead></thead><template><tr></tr></template><tr></tr><tfoot></tfoot></template>

Do we need additional tests (for misnested templates, etc)?

[Adam Klein]

Comment on attachment 179530 [details]
Patch with better ASSERTs

View in context: https://bugs.webkit.org/attachment.cgi?id=179530&action=review

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2517
>> +#if ENABLE(TEMPLATE_ELEMENT) && !ASSERT_DISABLED
> 
> Why is this disabled in release?

I had this here since only an ASSERT depends on the fact that the vector is cleared before destruction. I was worried about performance, but I guess we'll take the hit anyway when we destroy the tree builder, so I may as well remove the disable-in-release thing.

>> LayoutTests/html5lib/resources/template.dat:847
>> +<body><template><thead></thead><template><tr></tr></template><tr></tr><tfoot></tfoot></template>
> 
> Do we need additional tests (for misnested templates, etc)?

There are already some mis-nested examples in this file. Did you have particular test cases you had in mind?

[Adam Klein]

Created attachment 180004 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180004 [details]
Patch for landing

Clearing flags on attachment: 180004

Committed r138059: <http://trac.webkit.org/changeset/138059>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(In reply to comment #12)
> The review tool seems to have lost all my comments!?!

See https://bugs.webkit.org/show_bug.cgi?id=105252 .  I've seen it too, but we need repro steps.