Bug 65595 – <iframe> inside shadow tree should be invisible from the host document.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

<iframe> in a shadow tree have some problems for now.

 - hide shadow frames from windows.frames
 - Loading in a shadow frame shouldn't update the history.

[Kent Tamura]

Created attachment 102749 [details]
Patch

[Hajime Morrita]

Comment on attachment 102749 [details]
Patch

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

Another idea is to maintain a separate list for shadow frame.

> Source/WebCore/page/FrameTree.cpp:26
> +#include "HTMLFrameOwnerElement.h"

Is this required?

> Source/WebCore/page/FrameTree.cpp:70
> +    while (frame->isInShadowTree())

is fram always non-null?

> Source/WebCore/page/FrameTree.cpp:346
> +    if (frame)

Do we need this check?

> Source/WebCore/page/FrameTree.h:55
> +        // The number of child frames excluding shadow frames.

This is obvious from the code.

[Roland Steiner]

AFAICT, with this patch one cannot use firstChild(), nextChild(), previousSibling and nextSibling() within shadow trees at all. Rather than skipping frames where isInShadowTree() is true, it might be better to skip frames where the treeScope is different (?).

Also, in case the HTML of an <iframe> within a shadow tree includes itself an <iframe>, isInShadowTree() for the nested <iframe> would be false, and the frame thus visible. However, since the nested <iframe> got included by a shadow tree, this seems to run counter to the intent of the patch (?).

In the long run I wonder if it wasn't possible to have frame trees at the TreeScope level instead.

[Kent Tamura]

Comment on attachment 102749 [details]
Patch

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

Thank you for reviewing.

> Another idea is to maintain a separate list for shadow frame.

It hink the current idea is better to maintain.  We might want to change the behavior in the future so that loading for shadow iframes updates the browser history.

>> Source/WebCore/page/FrameTree.cpp:26

> 
> Is this required?

It's not required.  Removed.

>> Source/WebCore/page/FrameTree.cpp:70
>> +    while (frame->isInShadowTree())
> 
> is fram always non-null?

frame can be null.  But it's safe to call 0->isInShadowTree().

>> Source/WebCore/page/FrameTree.cpp:346
>> +    if (frame)
> 
> Do we need this check?

It's not needed.  frame is always non-0 in this case.
Fixed.

>> Source/WebCore/page/FrameTree.h:55
>> +        // The number of child frames excluding shadow frames.
> 
> This is obvious from the code.

Removed.

[Hajime Morrita]

Comment on attachment 102749 [details]
Patch

Could you add test for dynamic frame tree modification like
- from main document to shadow tree (and vice versa)
- from a shadow tree to another shadow tree
?

[Kent Tamura]

(In reply to comment #3)
> AFAICT, with this patch one cannot use firstChild(), nextChild(), previousSibling and nextSibling() within shadow trees at all. Rather than skipping frames where isInShadowTree() is true, it might be better to skip frames where the treeScope is different (?).

It's reasonable.  I need to investigate what we should do for Frame::ownerElement() == 0.

> Also, in case the HTML of an <iframe> within a shadow tree includes itself an <iframe>, isInShadowTree() for the nested <iframe> would be false, and the frame thus visible. However, since the nested <iframe> got included by a shadow tree, this seems to run counter to the intent of the patch (?).

In this case, a deeper <iframe> might update the browser history.  I'll make a test and check the behavior.

[Kent Tamura]

(In reply to comment #5)
> (From update of attachment 102749 [details])
> Could you add test for dynamic frame tree modification like
> - from main document to shadow tree (and vice versa)
> - from a shadow tree to another shadow tree
> ?

Do they make sense for the component model?
If such cases can be made only by window.internals, I don't think we should take care of them.

[Alexey Proskuryakov]

What's the use case for having an iframe in a shadow tree? Can we just disallow them instead?

[Roland Steiner]

(In reply to comment #8)
> What's the use case for having an iframe in a shadow tree? Can we just disallow them instead?

iframes in shadow tree have been discussed as one way to achieve strong isolation between the document and (parts of) a component.

[Kent Tamura]

(In reply to comment #8)
> What's the use case for having an iframe in a shadow tree? Can we just disallow them instead?

A patch in Bug 53961 uses it to inject a lot of style sheet and JavaScript code into a shadow tree.
I think we have no other ways to execute JavaScript code in shadow trees without polluting the global context.

[Dimitri Glazkov (Google)]

(In reply to comment #10)
> (In reply to comment #8)
> > What's the use case for having an iframe in a shadow tree? Can we just disallow them instead?
> 
> A patch in Bug 53961 uses it to inject a lot of style sheet and JavaScript code into a shadow tree.
> I think we have no other ways to execute JavaScript code in shadow trees without polluting the global context.

That sounds like a massive hack. I know you really want that calendar to land, but shouldn't we try to solve this problem properly?

[Hajime Morrita]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > What's the use case for having an iframe in a shadow tree? Can we just disallow them instead?
> > 
> > A patch in Bug 53961 uses it to inject a lot of style sheet and JavaScript code into a shadow tree.
> > I think we have no other ways to execute JavaScript code in shadow trees without polluting the global context.
> 
> That sounds like a massive hack. I know you really want that calendar to land, but shouldn't we try to solve this problem properly?
This might be a great opportunity to think about isolation:
If you want to create components in non intrusive way, what do we need?
This sounds worth to investigate.

[Alexey Proskuryakov]

I thought that the whole purpose of shadow trees was to provide isolation. If we need iframes for that, we could just as well remove shadow tree support and use iframes everywhere, couldn't we? :)

[Kent Tamura]

(In reply to comment #11)
> > A patch in Bug 53961 uses it to inject a lot of style sheet and JavaScript code into a shadow tree.
> > I think we have no other ways to execute JavaScript code in shadow trees without polluting the global context.
> 
> That sounds like a massive hack. I know you really want that calendar to land, but shouldn't we try to solve this problem properly?

Yes, I really want to make the progress of the calendar picker.
I don't think implementing "the proper way" will be done in a few months, and the iframe-in-shadow might be needed anyway.

[Kent Tamura]

Created attachment 103051 [details]
Patch 2

[WebKit Review Bot]

Comment on attachment 103051 [details]
Patch 2

Attachment 103051 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9321002

[Kent Tamura]

Created attachment 103053 [details]
Patch 3

Include Document.h for Chromium

[WebKit Review Bot]

Comment on attachment 103053 [details]
Patch 3

Attachment 103053 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9306789

New failing tests:
fast/frames/layout-after-destruction.html

[Kent Tamura]

Created attachment 103059 [details]
Patch 4

Fix a re-parenting bug

[Alexey Proskuryakov]

I still don't understand what the plan is here. Having iframes in shadow trees is a long term liability, and in the end, they should probably not be supported at all.

[Dimitri Glazkov (Google)]

(In reply to comment #20)
> I still don't understand what the plan is here. Having iframes in shadow trees is a long term liability, and in the end, they should probably not be supported at all.

Having iframes in shadow DOM is not a problem in general. None of the component model proposals currently in existence place any limitations like this.

However, using iframes in the context of 53961 definitely seems like a hack. If what you're looking for is a global JS context (which I don't think you do -- you could just use closure), then we should do this explicitly for JS running in a component. If you're trying to avoid stylesheet pollution, then use scoped styles.

Does this make sense?

[Sam Weinig]

(In reply to comment #0)
> <iframe> in a shadow tree have some problems for now.
> 
>  - hide shadow frames from windows.frames
>  - Loading in a shadow frame shouldn't update the history.

Where do these rules come from?

[Kent Tamura]

(In reply to comment #21)
> However, using iframes in the context of 53961 definitely seems like a hack. If what you're looking for is a global JS context (which I don't think you do -- you could just use closure), then we should do this explicitly for JS running in a component. If you're trying to avoid stylesheet pollution, then use scoped styles.


I confirmed <script> in a shadow tree worked.  Do you have any idea how to access the shadow tree from the script in the shadow tree?  We can't use document.getElementById(), etc.

[Kent Tamura]

(In reply to comment #22)
> >  - hide shadow frames from windows.frames
> >  - Loading in a shadow frame shouldn't update the history.
> 
> Where do these rules come from?

They are my opinions.  Others might object to them.

[Roland Steiner]

(In reply to comment #22)
> (In reply to comment #0)
> > <iframe> in a shadow tree have some problems for now.
> > 
> >  - hide shadow frames from windows.frames
> >  - Loading in a shadow frame shouldn't update the history.
> 
> Where do these rules come from?

In my understanding these are a natural consequence of the isolation discussion of components: if a component is supposed to be isolated from the page by default, then the page should have no way of accessing those iframes unless the component opens them up. Conversely, the component should not "pollute" the page's history by virtue of it utilizing iframes.

[Kent Tamura]

Comment on attachment 103059 [details]
Patch 4

I'm trying another way to implement the calendar picker.

[Dominic Cooney]

Now that ShadowRoot is exposed to script, ClusterFuzz demonstrated a crash adding an IFRAME to a ShadowRoot <http://code.google.com/p/chromium/issues/detail?id=113167>.

[Hajime Morrita]

Created attachment 128159 [details]
Patch

[Hajime Morrita]

This change just hides iframes from accessor on document. History stuff is out of scope.
I don't think we need to hide navigation in shadowed iframes because
we don't do that for nested iframes.

The concept of shadow DOM is further clarified since the last patch was written. 
<iframe> in shadow is no longer a big deal.

[Dimitri Glazkov (Google)]

Comment on attachment 128159 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This change introduced FrameTree::documentChild() and

"introduces"

> Source/WebCore/ChangeLog:10
> +        frame lookup. Using these, now the named accessor and the indexed

remove "now" here ...

> Source/WebCore/ChangeLog:11
> +        acceccor on Document, and Window.length is TreeScope aware. They

and replace "is" with "are now".

> Source/WebCore/page/Frame.cpp:1128
> +bool Frame::isScopedBy(TreeScope* scope) const

isScopedby -> inScope

> Source/WebCore/page/FrameTree.h:57
>          Frame* previousSibling() const { return m_previousSibling; }
>          Frame* firstChild() const { return m_firstChild.get(); }
>          Frame* lastChild() const { return m_lastChild; }

Are these guys still being used?

> Source/WebCore/page/FrameTree.h:78
> +        Frame* documentChild(unsigned index) const;

I think the name is a bit unintuitive. It requires you to know that Document is a TreeScope.

Also, the purpose of scoped* functions is limited to just supplying results for document* functions. Do we need the complete set of the two? Can we collapse them into one set, called "scopedChild"?

[Hajime Morrita]

Created attachment 128328 [details]
Patch

[Hajime Morrita]

Comment on attachment 128159 [details]
Patch

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

Hi Dimitri, thanks for reviewing this! I addressed your point. Could you have another round?

>> Source/WebCore/ChangeLog:8
>> +        This change introduced FrameTree::documentChild() and
> 
> "introduces"

fixed.

>> Source/WebCore/ChangeLog:10
>> +        frame lookup. Using these, now the named accessor and the indexed
> 
> remove "now" here ...

removed...

>> Source/WebCore/ChangeLog:11
>> +        acceccor on Document, and Window.length is TreeScope aware. They
> 
> and replace "is" with "are now".

... and fixed.

>> Source/WebCore/page/Frame.cpp:1128
>> +bool Frame::isScopedBy(TreeScope* scope) const
> 
> isScopedby -> inScope

fixed.

>> Source/WebCore/page/FrameTree.h:57
>>          Frame* lastChild() const { return m_lastChild; }
> 
> Are these guys still being used?

Yes. People iterate over frame children for example in the history module.

>> Source/WebCore/page/FrameTree.h:78
>> +        Frame* documentChild(unsigned index) const;
> 
> I think the name is a bit unintuitive. It requires you to know that Document is a TreeScope.
> 
> Also, the purpose of scoped* functions is limited to just supplying results for document* functions. Do we need the complete set of the two? Can we collapse them into one set, called "scopedChild"?

Sounds good. done.

[Dimitri Glazkov (Google)]

Comment on attachment 128328 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This change introduces FrameTree::documentChild() and
> +        FrameTree::documentChild(), which can be used for scope-aware

Whoops! Still mentions documentChild!

[Hajime Morrita]

Created attachment 128344 [details]
Patch

[Hajime Morrita]

(In reply to comment #33)
> 
> Whoops! Still mentions documentChild!
Whoaaaa. My text editor searched only under page/ and binding/ :-/

[Hajime Morrita]

> > Whoops! Still mentions documentChild!
> Whoaaaa. My text editor searched only under page/ and binding/ :-/
And bots are red... I need to fix them.

[Hajime Morrita]

Created attachment 128347 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 128347 [details]
Patch

r=me, make sure bots are green before landing.

[Hajime Morrita]

Created attachment 128377 [details]
Patch for landing

[Hajime Morrita]

Comment on attachment 128377 [details]
Patch for landing

Waiting EWS greeness.

[WebKit Review Bot]

Comment on attachment 128377 [details]
Patch for landing

Clearing flags on attachment: 128377

Committed r108700: <http://trac.webkit.org/changeset/108700>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The test added by this patch is failing on Mac port:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r109929%20(37823)/fast/dom/shadow/iframe-shadow-pretty-diff.html

There are a bunch of other tests that have been failing for the same missing WebKitShadowRoot error :( Could you guys skip all of those on Mac port?