templating icon indicating copy to clipboard operation
templating copied to clipboard

processContent( ) functor which injects a <template> results in a stack overflow.

Open kevmeister68 opened this issue 8 years ago • 19 comments

If using an @processContent decorator on a custom element, and the processContent functor injects a template tag, then returns true, then Aurelia will crash with a stack overflow.

Simple example:

@customElement( "my-widget" )
@processContent( MyWidget.processContent )
export class MyWidget
{
   static processContent( compiler: ViewCompiler, resources: ViewResources, node: HTMLElement, instruction: ( BehaviorInstruction & any ) ): Boolean
   {
      // NOTE: createChildElement is a helper that calls document.createElement then parent.appendChild.

      let child: HTMLElement = DOMHelper.createChildElement( "template", node );
      let child2: HTMLElement = DOMHelper.createChildElement( "p", child );

      child2.appendChild( document.createTextNode( "small message" ) );

      return true;
   }

I have also tried the alternate way of making the "child node" of the template element a DocumentFragment, because I have noticed that if I specify a template as content to my widget, then it appears with the "node" passed into the processContent functor, like a Document fragment. For example:

<my-widget>
   <template replace-part="greeting">
      <p>Hello</p>
   </template>
</my-widget>

All help greatly appreciated.

kevmeister68 avatar Sep 12 '16 23:09 kevmeister68

This is odd. Can you create a gist or a simple CLI-based app that I can use to play around with it and investigate?

EisenbergEffect avatar Sep 30 '16 05:09 EisenbergEffect

Is this in IE? IE does not build template elements with a content property, which can cause binding to go into an infinite loop.

I had a similar problem with viewCompiler. The workaround is to use (Typescript has to use "as any")

    if (!FEATURE.htmlTemplateElement) {
        (FEATURE as any).ensureHTMLTemplateElement(childTemplate);
    }

Note aurelia-pal-browser only publishes createTemplateFromMarkup which takes markup input, but hidden ensureHTMLTemplateElement takes element input

rockResolve avatar Oct 03 '16 00:10 rockResolve

Yes, the problem is indeed in IE but I don't get an infinite loop I get a stack overflow (well, it's an infinite loop leading to a stack overflow). I have built a Gist (Gist.Run) but I have not been able to get it to crash, but Gist.Run does not appear to run on IE, so I've been testing in Chrome. I'll go back and test my actual crashing code then re-try in Chrome and see if there's a difference.

kevmeister68 avatar Oct 03 '16 08:10 kevmeister68

OK, I have confirmed that:

  • The Problem does not occur in Chrome.
  • The Problem is resolved in IE by using the ensureHTMLTemplateElement "fix".

For the sake of other readers, the correct way to do this is as follows:

let myTemplate = document.createElement( "template" );

// PUT ALL OF YOUR CONTENT INTO myTemplate, eg.
// myTemplate. appendChild( document.createTextNode( "Hello" ) );

// ensure the template element has a content property
if( !FEATURE.htmlTemplateElement )
   ( FEATURE as any ).ensureHTMLTemplateElement( myTemplate );

This is because ensureHTMLTemplateElement actually copies data from the child nodes of a template into a content property.

My first attempt at the above failed because I called ensureHTMLTemplateElement too soon, before adding content, as I was not aware that ensureHTMLTemplateElement was performing "structural adjustment" to the template.

Kudos to you @rockResolve.

kevmeister68 avatar Oct 10 '16 21:10 kevmeister68

I'm running into a similar problem when manually compiling a template in IE which contains nested template tags.

Using FEATURE.ensureHTMLTemplateElement or DOM.createTemplateFromMarkup prevents stack overflows, but the result isn't working correctly.

When I use FEATURE.ensureHTMLTemplateElement all elements within a <template> tag with bindings disappear from the result (nested template tags without bindings actually appear in the DOM).

When using DOM.createTemplateFromMarkup some tags appear, others don't, and I can't find any way of telling which will appear or why.

Either approach works fine in Chrome.

RomkeVdMeulen avatar Dec 22 '16 16:12 RomkeVdMeulen

@EisenbergEffect: A bit more info to help resolve this bug:

pal-browser polyfills the <template> element by copying its nodes into a document fragment then sets the content property of that element to the newly created document fragment. https://github.com/aurelia/pal-browser/blob/735cab53849d611e246b6ab1e52ea29421e7e160/src/html-template-element.js#L29

This is good as long as that node or one of its parent node isn't cloned. https://github.com/aurelia/templating/blob/78b26068a0b7ef4f30be778059721c854227850d/src/view-factory.js#L428

Cloning doesn't clone the "custom" content property so the resulting node is just a plain simple <template> without any polyfilled info.

This cloneNode should be moved to pal-browser to copy the custom template.content, or use a really heavy polyfill that overrides every native cloneNode to do this but that would be a bit risky.

zenorbi avatar Jan 10 '17 13:01 zenorbi

Here is a Gist demonstrating the problem: https://gist.run/?id=7a0db8c7adda539e9081e341341c59cd

You need to download the files and host it yourself as gist doesn't work in IE (missing Promise implementation).

zenorbi avatar Jan 12 '17 10:01 zenorbi

Thanks everyone for the info and @zenorbi thanks for the demo. I think I have a fix for this, could you please try it out and let me know if it works for you?

Here's a copy of @zenorbi's demo with the fix applied, on plunker (so we can run it in IE 11): https://plnkr.co/edit/JCAByeyLqzwtyxgL1A4I?p=preview

Here's the fix, which you can use right away while you're waiting for the aurelia libs to be updated:

import {ViewFactory} from 'aurelia-templating';
import {FEATURE} from 'aurelia-pal';

ViewFactory.prototype.standardCreate = ViewFactory.prototype.create;
ViewFactory.prototype.create = function(container, createInstruction, element) {
  if (!FEATURE.htmlTemplateElement && !this.template.__safeToCloneNode) {
    const templates = this.template.querySelectorAll('template');
    if (templates.length === 0) {
      this.template.__safeToCloneNode = true;
    } else {
      this.template.standardCloneNode = this.template.cloneNode;
      this.template.cloneNode = function(deep) {
        const clone = this.standardCloneNode(deep);
        if (deep) {
          const clonedTemplates = clone.querySelectorAll('template');
          let i = clonedTemplates.length;
          while (i--) {
            clonedTemplates.item(i).content = templates.item(i).content;
          }
        }
        return clone;
      };
      this.template.__safeToCloneNode = true;
    }
  }
  
  return this.standardCreate(container, createInstruction, element);
};

jdanyow avatar May 20 '17 14:05 jdanyow

@jdanyow The fix works great both in your plunker and in our internal project (tested with IE 11 and IE 10). Thank you for the hotfix code.

zenorbi avatar May 20 '17 17:05 zenorbi

Nice 👍 thanks for following up. I need to do some more testing and refactor such that it's all in aurelia-pal-browser but hopefully we can ship this fix soon.

jdanyow avatar May 20 '17 18:05 jdanyow

Any updates on this?

RomkeVdMeulen avatar Jul 17 '17 08:07 RomkeVdMeulen

We've tried @jdanyow's fix in our own project and are still having problems. Like I said above: by applying ensureHTMLTemplateElement the stack overflow goes away, but templates with bindings disappear from the result. This fix doesn't seem to prevent that. Logging shows that by the time this line is reached:

clonedTemplates.item(i).content = templates.item(i).content;

the templates.item(i).content already contains a template from which all content and template attributes have disappeared. Something like <template class="au-target" au-target-id="27"></template>. We'll investigate further though it's pretty tough going.

RomkeVdMeulen avatar Aug 22 '17 08:08 RomkeVdMeulen

I've released an update to PAL so that we now have DOM.createTemplateElement() which will create an element and ensure that the created template is polyfilled correctly. That can be used to safely create template elements on the fly without having to call FEATURE.ensureHTMLTemplateElement or even know that that exists.

@jdanyow Do you have another fix that needs to be contributed elsewhere?

EisenbergEffect avatar Aug 23 '17 04:08 EisenbergEffect

@EisenbergEffect Would that work? My understanding was that ensureHTMLTemplateElement copies over child nodes into the 'content' property once it's called. If it's called before the child nodes are added, there's no point, is there?

The createTemplateFromMarkup function seems to do this: parse the given markup, make sure it's wrapped in a <template> tag, and then calling ensureHTMLTemplateElement on it.

RomkeVdMeulen avatar Aug 24 '17 08:08 RomkeVdMeulen

If you create a template element with the new helper, you would just add the nodes to its content property. If you need to create a child template, you would use the same helper to create the child template as well, and just add it to the parent template's content property. It should all work.

EisenbergEffect avatar Aug 24 '17 08:08 EisenbergEffect

Ah, I see. Thank you for clearing that up.

The one thing that you couldn't do with the new helper would be to create a template element and set its content to some markup / nodes that might or might not contain child templates, but I guess in that case you should be using createTemplateFromMarkup anyway.

RomkeVdMeulen avatar Aug 24 '17 09:08 RomkeVdMeulen

@EisenbergEffect Am I missing something that's left to be done or this can be closed?

StrahilKazlachev avatar Sep 17 '17 15:09 StrahilKazlachev

I think we need to integrate this in the view factory: https://github.com/aurelia/templating/issues/460#issuecomment-302878158

jdanyow avatar Sep 17 '17 16:09 jdanyow

@jdanyow If you don't have time, I think I should have this week, should I do take it on?

StrahilKazlachev avatar Sep 17 '17 16:09 StrahilKazlachev