Last Comment Bug 80713 - Need a way to specify an auto-height (size) for an IFRAME such that the frame is given the full height of the contained content (moz-seamless, part of seamless iframes)
: Need a way to specify an auto-height (size) for an IFRAME such that the frame...
Status: NEW
[needs owner]
:
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: Trunk
: All All
: -- normal with 51 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 184155 748945
Blocks: 9942 31052 82280 241197 msgreadertracker 464309 77539 449691 631218
  Show dependency treegraph
 
Reported: 2001-05-14 09:00 PDT by Marc Attinasi
Modified: 2016-05-21 05:55 PDT (History)
92 users (show)
roc: wanted1.9.1-
shaver: blocking1.8.1-
dveditz: sec‑review?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (-w, for review only) (33.91 KB, patch)
2005-02-24 15:00 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
Shows that the height on root elements is ignored (157 bytes, text/html)
2005-02-24 15:03 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
Content for iframes (1.64 KB, text/html)
2005-02-24 15:04 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
auto-height IFRAME testcase (391 bytes, text/html)
2005-02-24 15:06 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
full patch (41.24 KB, patch)
2005-02-24 15:07 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
testcase for content that depends on viewport height (174 bytes, text/html)
2005-03-30 16:01 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
master testcase (315 bytes, text/html)
2005-03-30 16:03 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
full patch, v2 (45.50 KB, patch)
2005-03-30 16:16 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
diff -w of full patch v2 (38.75 KB, text/plain)
2005-03-30 16:22 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review-
dbaron: superreview-
Details
completely updated patch (48.73 KB, patch)
2008-08-26 17:30 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review-
dbaron: superreview-
Details | Diff | Review
Updated patch (43.19 KB, patch)
2011-08-17 13:15 PDT, Jonathan Protzenko [:protz]
no flags Details | Diff | Review
Updated updated patch (46.58 KB, patch)
2011-08-18 19:59 PDT, Jonathan Protzenko [:protz]
roc: feedback+
Details | Diff | Review
Updated patch v3 (47.51 KB, patch)
2011-08-19 15:43 PDT, Jonathan Protzenko [:protz]
dbaron: review-
Details | Diff | Review
Derotted patch (46.61 KB, patch)
2012-06-09 19:37 PDT, Joshua Cranmer [:jcranmer]
no flags Details | Diff | Review

Description Marc Attinasi 2001-05-14 09:00:09 PDT
Specifying an <iframe> with width:100% and height:auto would help in the mail
for multi-part HTML messages, which right now can be messy wrt <base href> and
other markup tags/constructs.

Spun-off of bug 77539
Comment 1 Marc Attinasi 2001-05-14 09:01:26 PDT
0.9.1 and P2 since the block-bug is pretty common and pretty nasty.
Comment 2 Chris Waterson 2001-05-14 09:34:59 PDT
cc'ing hyatt n' hixie. Is there a way that exists now to construct an IFRAME 
that will auto-size its height to the height of its content, and _never_ 
display a scrollbar?
Comment 3 Scott MacGregor 2001-05-14 10:50:13 PDT
Marc, I don't think we have to convert inlined parts in a mail message to
iframes with this auto size feature for .9.1. I think it would be too much work
on your end and on mine. So feel free to push this off till later. I was
planning on coming up with a work around in 77539. I think this is the direction
we want to go in the near future though.
Comment 4 Marc Attinasi 2001-05-14 11:19:09 PDT
Great, thanks. Moving to 0.9.2
Comment 5 Hixie (not reading bugmail) 2001-05-19 05:36:15 PDT
The standards are silent on this issue. I don't know of a way to do it with our
implementation as is.
Comment 6 Marc Attinasi 2001-05-31 14:01:24 PDT
by mandate, moving non-crashers and non-datalossers outward.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2003-04-22 00:53:41 PDT
.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-02-24 15:00:08 PST
Created attachment 175482 [details] [diff] [review]
fix (-w, for review only)

Inspired by Gerv's blog post
(http://weblogs.mozillazine.org/gerv/archives/007610.html) I've taken a crack
at this. It wasn't really very hard. I simply make auto-height IFRAMEs, whose
subdocument's root element is a block, have an intrinsic height which is the
height the block would have if it was 'height:auto'. If the root element
actually has a height set, then we ignore it, which is probably wrong, but then
we ignore it in normal situations too, currently. We need to start respecting
'height' on root elements all at once.

Basically there are two things we need to do:

-- Get the IFRAME's scrolled block's auto-height back out to
nsSubDocumentFrame::Reflow, which if necessary loops around once to re-reflow
with the new intrinsic height. The tricky thing is that the scrolled block's
computed height (and therefore its desired height) is always set to the current
scrollport height. So I added a way in nsHTMLReflowMetrics for a frame to
return what its height would have been if it had been 'height:auto', and made
blocks set it. We then store this on the scrollable frame where
nsSubDocumentFrame::Reflow can pick it up.

-- When something changes in the subdocument, its intrinsic height might
change. So we have to notify the nsSubDocumentFrame, which will reflow itself
if necessary.

I'll attach a testcase which works just great.

Comments? One thing I'm not sure about is how badly this will break Web sites
that rely on the default height of an IFRAME being fixed.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-02-24 15:03:39 PST
Created attachment 175483 [details]
Shows that the height on root elements is ignored
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-02-24 15:04:34 PST
Created attachment 175484 [details]
Content for iframes
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-02-24 15:06:37 PST
Created attachment 175485 [details]
auto-height IFRAME testcase
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-02-24 15:07:53 PST
Created attachment 175486 [details] [diff] [review]
full patch
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-02-24 19:04:24 PST
Note that this feature might be a security issue because you could determine the
height of any HTML content you can load in an IFRAME. I guess I should add some
kind of access check so that you only get intrinsic sizing if the IFRAME's
document is in the same domain as its parent.
Comment 14 Max Kanat-Alexander 2005-02-24 19:35:19 PST
I think that would make the feature considerably less useful, though, since
iframes *are* used to display cross-domain content.

What's the actual *security* risk? We can access any other part of the iframe's
internal DOM besides its height, here, and we can't even get to that except that
we know now the scrollHeight of the iframe (which tells us the content height of
the iframe).
Comment 15 Gervase Markham [:gerv] 2005-02-25 01:49:36 PST
roc: thanks very much for taking up the baton here :-)

I'm not convinced I can think of an exploit for the potential security issue.
You would need to find a site where two different heights of a page leaked one
bit of information which was useful to an attacker. Perhaps they could find out
if a user was logged into a site or not (error message has different height to
"welcome" page). Is that information alone risky? After all, if they wanted to
try a session-riding operation on said site, they'd just try it. (Yes, we need
to look at what we can do about session riding. That's another issue.)

Gerv
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-02-25 05:07:55 PST
So... with a percentage-sized root in the child document, won't this lead to
incremental reflow bugs?  I don't see us trying a reflow of the child document
with NS_UNCONSTRAINEDSIZE for the initial containing block computed height
(which is what we should probably be doing, though that will effectively turn
all percentage heights in the subframe into auto heights...)
Comment 17 Hixie (not reading bugmail) 2005-02-25 08:15:34 PST
> I'm not convinced I can think of an exploit for the potential security issue.

I can't think of an exploit either, but it seems on par with the :link/:visited
leak, for what it's worth. And probably has the same solution (namely, make it
not happen. You can always work out what the height of an element is, just by
checking the position of a later element or similar).
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-02-28 11:29:33 PST
I think comment 13 is a security issue that we need to worry about.
Comment 19 Gervase Markham [:gerv] 2005-02-28 14:52:37 PST
dbaron: could you go into more detail? What sort of exploits would this enable?
What do you think of Hixie's view in comment 17?

Gerv
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-11 01:23:58 PST
(In reply to comment #16)
> So... with a percentage-sized root in the child document, won't this lead to
> incremental reflow bugs?  I don't see us trying a reflow of the child document
> with NS_UNCONSTRAINEDSIZE for the initial containing block computed height
> (which is what we should probably be doing, though that will effectively turn
> all percentage heights in the subframe into auto heights...)

Yeah, that requires a bit more work.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-11 01:26:13 PST
I can't think of an exploit either, but I bet some fool out there has designed a
site that leaks information in the height of its documents. The way to fix this
is to do a same-origin check and force the default intrinsic size on IFRAMEs
with a different origin to the host.
Comment 22 Gervase Markham [:gerv] 2005-03-12 04:30:44 PST
Sounds good, although wouldn't that be the first time the layout of a page was
affected by the origin of some of its components? There's a risk of confusing
web developers, and having their sites lay out differently on disk (or partly on
disk) to when uploaded.

Gerv
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-21 14:12:31 PST
Yeah, that's possible, but I can't think of anything better.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-30 14:59:55 PST
(In reply to comment #22)
> Sounds good, although wouldn't that be the first time the layout of a page was
> affected by the origin of some of its components? There's a risk of confusing
> web developers, and having their sites lay out differently on disk (or partly on
> disk) to when uploaded.

I'm guessing that if a developer has a page on disk that links to another page
on disk, then they're intended to be uploaded to the same site ... otherwise the
link is broken anyway.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-30 16:01:31 PST
Created attachment 179117 [details]
testcase for content that depends on viewport height
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-30 16:03:28 PST
Created attachment 179118 [details]
master testcase

This tests the same-origin check (should fail for Google, succeed for Bugzilla)
and also shows that if the frame's content height depends on viewport height,
we don't collapse in a screaming heap.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-30 16:16:56 PST
Created attachment 179122 [details] [diff] [review]
full patch, v2

Updated to trunk. CheckSameOriginURI added as discussed. Arranged for
auto-height IFRAMEs to not set the containing block height for the root block
frame. (Note that if the IFRAME's root element is not a block, then we don't
set the unstretched autoheight on the scrollframe, so no intrinsic sizing
happens.) I think this is ready to roll.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-30 16:22:37 PST
Created attachment 179123 [details]
diff -w of full patch v2

I don't know who wants to review this, if anyone :-).

There's a question of whether we should take document.domain into account when
we do the same origin check. I guess that's a possible enhancement.

If you browse within one of these things, the display jumps around because at
times the new document is empty so the IFRAME falls back to zero height
(actually slightly nonzero because of default margins). I guess that could be
fixed later.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-03-30 16:44:32 PST
It does like this would be more useful if we could make the security check
asymmetric --- so that a container could host an IFRAME with intrinsic height,
but script in the IFRAME was not permitted to access the container. I don't know
how to do that, or even if it's possible with our security infrastructure.
Comment 30 Gervase Markham [:gerv] 2005-03-31 01:23:00 PST
My Content-Restrictions proposal would enable is to prevent script in the iframe
accessing the parent when they are both in the same domain. (Of course, if they
are in different domains, it can't anyway, because of same-origin.) 

http://weblogs.mozillazine.org/gerv/archives/007821.html

It's not a coincidence that it does so and that I'm keen on this bug :-)

Gerv
Comment 31 Gervase Markham [:gerv] 2005-09-22 02:15:23 PDT
roc: is there any chance of progress here? Are the security issues solved to
your satisfaction?

Gerv
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-11-15 20:38:35 PST
I think we should push this in.
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-01-11 15:15:57 PST
OK, do you have a version merged to trunk, or should I review the current patch?

I'm really not happy with the idea of making layout depend on cross-site scripting restrictions, though.  Authors frequently develop pages on test servers or on their filesystem and then publish the pages.  Cross-site scripting restrictions already through a big wrench into such development methods; I'm quite hesitant to make things even harder for authors to understand and to test.
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-11 15:22:57 PST
(In reply to comment #33)
> OK, do you have a version merged to trunk, or should I review the current
> patch?

I can merge to trunk for you.

> I'm really not happy with the idea of making layout depend on cross-site
> scripting restrictions, though.  Authors frequently develop pages on test
> servers or on their filesystem and then publish the pages.  Cross-site
> scripting restrictions already through a big wrench into such development
> methods; I'm quite hesitant to make things even harder for authors to
> understand and to test.

I agree. But I can't think of a better way forward.
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-01-12 14:10:24 PST
(In reply to comment #34)
> I can merge to trunk for you.

Probably better to review a merged patch, then, since it'll have to be merged eventually.

> I agree. But I can't think of a better way forward.

Well, I guess what I'm not convinced of is that doing it at all is worth those disadvantages.
Comment 36 Gervase Markham [:gerv] 2006-01-30 09:00:20 PST
dbaron: currently, I would assert, no-one uses HTML in iframes with no size set on the web because of the current behaviour. So, if people start doing so, they will just have to understand the behaviour. If it doesn't work how they expect, what have they lost? Before, they couldn't do it at all.

Gerv
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-03-31 11:26:55 PST
If we do same-origin checks on documents, we should really use CheckSameOriginPrincipal, not CheckSameOriginURI....
Comment 38 Hixie (not reading bugmail) 2006-03-31 17:32:21 PST
We're assuming that authors don't rely on the height of an IFRAME being 150px, I take it?
Comment 39 Gervase Markham [:gerv] 2006-03-31 17:49:39 PST
Yes. Do you think that's an unreasonable assumption?

Is the default different in other browsers?

Gerv
Comment 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-03-31 17:51:14 PST
I thought the idea here was that it was being done so that it was not the default behavior, but could be obtained with 'height: auto' (which would not be the default).
Comment 41 Hixie (not reading bugmail) 2006-03-31 22:04:10 PST
dbaron: 'height: auto' is the default for <object> (and pretty much has to be). It currently is also the default for <iframe>; that, I suppose, could change.

Gerv: I don't know if it's a good assumption or not. It's an assumption.
Comment 42 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-01 01:46:17 PST
Could we add a rule to html.css like:

iframe:not([height]) {
  height: 100px;
}

(or whatever the default height currently is)
Comment 43 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-04-02 14:32:19 PDT
Sure, but why bother with the :not([height]) bit?  If height is set to a number, it would override the ua.css rule anyway.
Comment 44 Jonas Sicking (:sicking) PTO Until July 5th 2006-04-02 18:47:04 PDT
Even if it is specified as an attribute? I don't remember the cascade rules offhand.
Comment 45 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-04-02 19:02:54 PDT
Presentational attributes override non-important UA and user rules.
Comment 46 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-04-18 10:33:53 PDT
blocking-, renominate when there's consensus on web-compat and XSS issues.
Comment 47 Gervase Markham [:gerv] 2006-05-24 07:56:35 PDT
So it looks like the way forward is to change the patch to add the following to html.css:

iframe {
  height: 150px; # The current default
}

This makes people have to set height:auto explicitly on <iframe> in order to get the new behaviour; existing iframes stay as they are.

Opera has the same default size for <iframe> as we do (150px). I don't have easy access to Konqueror/Safari or IE.

Hixie said:
> 'height: auto' is the default for <object> (and pretty much has to be).

Then surely failing to do an auto height on an <object type="text/html"> (as this bug requests) is definitely a bug?

Gerv
Comment 48 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-05-24 08:00:36 PDT
(In reply to comment #47
> Opera has the same default size for <iframe> as we do (150px). I don't have
> easy access to Konqueror/Safari or IE.

IE6 uses 150px as well.
Comment 49 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-03-23 11:55:48 PDT
Comment on attachment 179123 [details]
diff -w of full patch v2

So, I should have marked this ages ago, but:

The fact that this introduces new behavior without adding a new way to ask for that behavior pretty much guarantees that it will break CSS spec conformance, and probably also break compatibility with some Web pages.  If we want to do this, we need to add a new value to request this behavior (for example, using a new -moz-* value for 'height').

I'm also not really happy about adding cross-site checks in layout.  That's another reason I'd like to require that authors have to explicitly request this behavior -- I'd rather not have the default case be something that changes when pages are moved between sites.

I think the way to move forward here is to agree on what the behavior should be ("if authors do X, then Y happens") and then update the patch.

Sorry for leaving this request sitting around for so long.
Comment 50 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-03-23 11:59:55 PDT
It would also be nice to know what height we're actually using.  roc made comments above about ignoring 'height' on the root element.  Do we consider in-flow content?  Floats, since the root establishes a block formatting context?  Absolutely positioned content?  Fixed positioned content?  Overflowing content due to negative margins or explicit heights?
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-03-23 15:09:42 PDT
Adding a new CSS value for 'height' seems tricky since it would only be applicable to this one element. What if we added a new attribute to IFRAME instead, say autoheight="true"? Then I would propose the following spec:

If autoheight="true" and the outer document is permitted to access the DOM of the inner document, the <iframe> element's intrinsic content height is set to the height of the child document's root element (the height of the first CSS box, if there is more than one). When autoheight="true" the height of the initial containing block for the child document is set to zero.

AFAIK CSS does not say anything special to the height of the root element's box --- backgrounds, borders and scrolling all propagate to the viewport/canvas --- so I think that works. It means that floats are included in the height calculation, but fixed and abs-pos elements, and other overflowing children, are not.

If that sounds like a good proposal I'll revisit the patch.
Comment 52 Gervase Markham [:gerv] 2007-03-26 08:59:04 PDT
Why did we decide that the CSS "height: auto" was unsuitable for triggering this behaviour?

Currently, according to Hixie in comment #41, "height: auto" is the default for <iframe>. That means that it's at least plausible that there are very few web pages out there which explicitly set "height: auto" on iframes. Because there's no point. It doesn't change anything.

However, the actual behaviour you get is a 150px fixed height <iframe>. So if we changed the default to do that in html.css, and did nothing else, the web would stay the same as today.

But that would free up the possibility of using height: auto (which, we suggest, no-one is using) to trigger the newly-implemented behaviour of, er, making the iframe automatically have the right height for its content (which seems sane semantically).

Where have I gone wrong? Or are we just concerned about how many pages might already be doing height: auto on <iframe>? Hixie, is that something you might be able to use Super Google Power to check?

Gerv
Comment 53 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-03-26 11:06:23 PDT
(1) the spec says so
(2) as I said above, I don't want this behavior to happen for the default value
Comment 54 Gervase Markham [:gerv] 2007-03-26 15:12:27 PDT
But our current behaviour (default to 150px high) isn't conformant to the spec. Is it? So at worst we'd be changing one non-conformance for another. And if we made height: auto actually do what one might expect it to do, we are arguably getting even closer to the spec.

As for your point 2), if we changed the default value to be height: 150px then it wouldn't happen for the default value! But then I guess that would be a spec violation, in that the spec says that "auto" is the default for the height of anything.

Gerv
Comment 55 Brad Fults 2007-04-23 18:11:00 PDT
I agree with Gerv -- it makes sense to keep violating the spec where other browsers agree and web pages might be depending on it (default height: 150px), but it also makes sense both semantically and for implementation that the new "automatically set your height to accommodate your content" behavior be specified in CSS as height: auto.
Comment 56 Gervase Markham [:gerv] 2007-05-11 07:18:04 PDT
It's far more important that this goes in than it stays stalled because of a dispute about the exact syntax. If dbaron wants a -moz-* value, fine. Let's use -moz-auto and be done with it. :-) roc?

Gerv
Comment 57 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-05-11 17:19:54 PDT
I'd suggest not -moz-auto. How about -moz-child-document instead? It's up to David really.
Comment 58 Gervase Markham [:gerv] 2007-05-15 04:15:55 PDT
The following question was asked on the what-wg mailing list:

"I see you have done some work to prevent reflow loops with percentage root heights > 100%, but how does your patch handle an iframe document that looks like this? (I can think of nastier testcases also, where "bottom" is embedded further down in the document)

<html>
<head>
</head>
<body>
<div style="position:absolute;bottom:-5px;">This will force a scrollbar on the document</div>
</body>
</html>"

and further:

":) The point I wanted to make (although not very clearly i admit) was that there are several concerns wrt behaviour that might need to be discussed before recommending this behaviour, such as content with %heights > 100%, %heights < 100% combined with other blocks, negative bottom/right alignment and maybe more such as relation between auto widths and auto heights."

Gerv
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-05-15 15:35:33 PDT
I don't think there's a problem here, given my definition in comment #51. The absolute positioned DIV overflows the root element but does not affect the size of the root element. In this particular example, assuming the UA style sheet has no margins or padding for the elements, the root element is size zero and so will be the IFRAME.

Also, we force the initial containing block to have zero height, so there's never a dependency loop between the positioning of abs-pos elements and the height of the initial containing block, no matter what diabolical markup you come up with.
Comment 60 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-02-19 16:10:44 PST
Maybe height: -moz-child-content?  (I could imagine wanting width: -moz-child-max-content, width: -moz-child-min-content, width: -moz-child-fit-content.)  I don't have strong opinions on the name, though.
Comment 61 Ben Bucksch (:BenB) 2008-04-09 05:07:56 PDT
> I don't have strong opinions on the name, though.

Good. Robert, can you make your pic, then, so that we can get it in?
This is quite a restriction in the platform, see e.g. bug 9942.
Comment 62 David Ascher (:davida) 2008-08-07 16:52:21 PDT
Nominating for wanted1.9.1.  From Thunderbird's POV, we don't care what the syntax is.
Comment 63 Ben Bucksch (:BenB) 2008-08-07 18:15:21 PDT
FYI, I talked with roc (and dbaron) on firefox summit about it and he said he wants to do it for 1.9.1. (Thanks!)
Comment 64 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-15 16:47:19 PDT
I propose that we trigger this behaviour using the HTML5 "seamless" attribute.

http://www.whatwg.org/specs/web-apps/current-work/#seamless

It won't be a full implementation of "seamless", obviously, but since sites have to fall back for UAs that don't implement "seamless" at all, it seems like doing this partial improvement should be safe.

There will have to be a same-origin restriction (or whatever the relaxation of that is when we allow chrome to link to anything).
Comment 65 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-19 22:18:11 PDT
After further discussion with Ian, we should not use the "seamless" attribute until we have a decent implementation of what he specced out there, which includes some pretty hard stuff like munging stylesheets and style inheritance. So I'll just use a mozSeamless attribute for now and then we can transition to seamless if/when we have an adequate implementation.
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-21 21:53:34 PDT
Since I wrote the original patch, jwatt implemented intrinsic sizing for <object> embedding SVG. So we just need to extend that somewhat.
Comment 67 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-22 02:47:32 PDT
The HTML5 spec tries to achieve seamless layout by setting the intrinsic width and height of the IFRAME. But this isn't really what's wanted, because for example setting the intrinsic height to something that depends on the actual width of the <iframe> means the height isn't really intrinsic anymore.

I think what we really want is more like what was in the previous patch, to not treat seamless iframes like replaced elements for layout. That means overriding ComputeSize to delegate the width computation to the root frame of the subdocument, but returning an unconstrained height. Then in Reflow we return the height of the subdocument's root frame as our actual height.
Comment 68 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-24 22:27:14 PDT
Right now I'm trying to deal with the problem that the fix for bug 396587 made us reflow the IFRAME subdocument in a post-reflow callback, because it can run script. But that's not the ideal time to do the reflow for seamless iframes; we'd really prefer to do it during nsSubdocumentFrame::Reflow or better still nsSubdocumentFrame::ComputeAutoSize, so we can get the document's root element's height and return it.

I can hack around it by storing the last returned root element height and in the post-reflow callback, after we've reflowed the subdocument, check if the height changed and if it did, ask the iframe to reflow again. (That should terminate because we don't allow the seamless subdocument layout to depend on the iframe's height.) But that seems quite inefficient because in a normal load we'll reflow the outer document, including placing all the iframes with the correct width but zero height, then we'll reflow all the subdocuments, then we'll reflow the outer document again to size the iframes to their correct heights.

Boris, could we partially back out the fix for bug 396587 and replace it with nsContentUtils script-blocking? I'm not really sure what script can run during the subdocument reflow ... onresize handlers?
Comment 69 Jonas Sicking (:sicking) PTO Until July 5th 2008-08-25 00:30:42 PDT
Yeah, I'm curious too how scripts could run. If we were firing untimely events, it would seems like the correct fix is to use NS_DispatchToCurrentThread or nsContentUtils::AddScriptRunner.

The latter would require that you audit all call stacks that can lead to the call and make sure they block scripts appropriately, though I think layout code should be in a pretty good state at this point.
Comment 70 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-08-25 08:32:18 PDT
> Yeah, I'm curious too how scripts could run.

Something like this:

nsSubDocumentFrame::Reflow calls
nsDocShell::GetPositionAndSize calls
nsDocument::FlushPendingNotifications

then you lose.  The testcase in this bug, for example, triggers an XBL constructor that removes the iframe node from the document.

Maybe what we really want is a flush-less version of GetPositionAndSize.  Or a way to not get anything, and just set the size without changing the position.  Or something.
Comment 71 Jonas Sicking (:sicking) PTO Until July 5th 2008-08-25 18:13:18 PDT
Would doing the whole "batch inserts, not notifications" thing help? Not actually sure that it would given XBLs involvement.
Comment 72 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-26 17:30:52 PDT
Created attachment 335644 [details] [diff] [review]
completely updated patch

Behaviour is activated by specifying a "moz-seamless" attribute on an <iframe> element. It is only in effect while the subdocument's principal is subsumed by the outer document's principal; this basically means they have the same origin, although chrome-privileged documents can seamlessly embed anything. It only works when the view manager trees are linked, which means that as things stand, chrome docshells cannot seamlessly embed content docshells.

The layout behaviour in HTML5 for "seamless" iframes
http://www.whatwg.org/specs/web-apps/current-work/#seamless
is a bit strange because it tries to redefine intrinsic size in a way that makes it depend on the container's layout, i.e., not intrinsic. So I did things a bit differently. Basically:
-- the intrinsic and min-intrinsic widths of a seamless iframe are taken from its subdocument's root element (including its border, padding and margins)
-- the width of a seamless iframe is the width the subdocument's root element would have in the same context (including its border, padding and margins)
-- the height of a seamless iframe is the height of the subdocument's root element (including its border, padding and margins)

Note that viewport scrollbars are NOT taken into account, so if they appear, they usually screw things up. So if you're using this feature, you want to suppress them. We could make "moz-seamless" automatically disable viewport scrollbars in the subdocument, but I haven't done that here.

The patch is not all that tricky. There's some stuff to propagate changes in the subdocument up to the iframe when necessary. It turns out to be necessary to notify on resize reflows of the subdocument, because they can be suppressed and reactivated at unpredictable times. We have to force the initial containing block height to 0 in nsHTMLReflowState. This patch subsumes some of the SVG intrinsic-size-change notification code, and actually fixes a bug in the process (see enabled SVG reftest).

The ickiest bit is probably the way we have to get the height of the subdocument, as already discussed. We have to reflow the subdocument in ReflowFinished, so I call FlushPendingNotifications there to make sure the reflow really happened and then get the height, and if it changed we have to reflow the iframe again.

To test the same-origin checking, I modified reftest.js to support cross-domain HTTP resource loads.
Comment 73 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-26 17:45:03 PDT
I'm not 100% sure if this will solve the mail-body problem for Thunderbird. The question is how to set the width of the iframe. There are a few design options:
-- force the iframe width to the width of the message pane, hide horizontal scrollbar. Then any horizontal overflow will be cut off.
-- force the iframe width to the width of the message pane, allow horizontal scrollbar in the iframe. Then the scrollbar will overlap the bottom of the mail body. (I guess you could hack in some padding there just in case.) It will also be a bit strange since the vertical scrollbar scrolls the body+headers and the horizontal scrollbar will scroll just the body and will be scrolled out of view when the body+headers are scrolled to the top.
-- set the iframe min-width to the iframe's min-intrinsic width and suppress subdocument scrollbars. There can still be overflowing content cut off, if there are elements with specified width and overflowing children, but otherwise the iframe will grow horizontally to fit its content. However, the subdocument will be laid out to the iframe's width, so for example one really wide image would make all lines in the message body that long. This is still probably the best option though.

To fix that last issue, we'd have to do some extra work so that the available width of the iframe's context is propagated down for the reflow of the subdocument.
Comment 74 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-26 17:47:32 PDT
(In reply to comment #72)
> -- the width of a seamless iframe is the width the subdocument's root element
> would have in the same context (including its border, padding and margins)
> -- the height of a seamless iframe is the height of the subdocument's root
> element (including its border, padding and margins)

Here I mean the 'auto' width and height. Of course it's still possible to control the width and height of a seamless iframe by styling it.

One difference between this behaviour and actually setting the intrinsic width and height is that moz-seamless iframes do not support the CSS 2.1 aspect-ratio behaviour for replaced elements.
Comment 75 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-08-27 16:25:22 PDT
In comment #73 when I wrote "message pane" I mean specifically the message *body* pane containing only the sandboxed message content.
Comment 76 Tony Mechelynck [:tonymec] 2008-08-29 18:29:59 PDT
[OT] (In reply to comment #75)
> In comment #73 when I wrote "message pane" I mean specifically the message
> *body* pane containing only the sandboxed message content.

Hm, maybe you should check bug 449691.
Comment 77 Dan Mosedale (:dmose) 2008-09-03 12:11:07 PDT
After discussion with roc on IRC, it looks like, in addition to this patch, Thunderbird will want option 3 from comment 73 for scrolling (ideally with the "to fix that last issue" changes as well).  We'll also need the ability to have embedded content docshells (viz paragraph 1, comment 72), since that's what the body pane is.
Comment 78 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-11 10:58:45 PDT
Comment on attachment 335644 [details] [diff] [review]
completely updated patch

+   * document (e.g. <object> containing SVG or a "seamless" iframe) then

The code only checks the latter case.  (Updating just the comment seems
ok.)

And in fact PresShell::ResizeReflow depends on the resulting frame being
an nsIFrameFrame.  Given that callers assume that, you should probably
document that in the header.

+  if (!f->GetParent() || checkAncestorDocument) {

Maybe it's worth commenting on why this condition is OK -- in
particular, that you need to pass aIntrinsicDirty up even if you broke
out of the second loop due to wasDirty, because aIntrinsicDirty could be
different.  However, you don't need this if aIntrinsicDirty == aResize
(the weakest possibility), and that's the exact condition under which
you skipped the loop in which checkAncestorDocument was set.  (Of
course, it almost seems like unnecessary complexity.)


Up to nsFrameFrame.cpp.

In nsSubDocumentFrame::ObtainIntrinsicSizeFrame, I tend to think that
the checks for SVG and for _moz_seamless should be the other way around;
the SVG behavior seems more powerful to me, so I think _moz_seamless
shouldn't weaken it.  I *think* -- I'm not sure I understand the
_moz_seamless behavior well enough, yet, though.

I'm almost tempted to say ComputeNonreplacedIntrinsicSize should use
aCBWidth where it currently uses aAvailableWidth, since the presence of
aAvailableWidth itself is almost a quirk (used to make things narrower
when next to floats).  Though perhaps it should stay as you have it now.

Next to the definition of mIgnoreSubdocResizeReflow, maybe add a comment
saying it's to suppress recursion?  Or maybe call the variable
mDoingSubdocResize?


The FlushPendingNotifications call inside ReflowFinished scares me a
bit, although I don't know of anything problematic that it would cause
right *now*.  It means, however, that we have flushes that go both up
and down the tree, which means we're not far from getting into a
situation where we can get flushes recurring into themselves in some
cases by going from parent to child to parent (or the other way around).

(We're ok because nsDocument::FlushPendingNotifications calls up to the
parent for Flush_Style or higher, nsPresShell::FlushPendingNotifications
flushes the document only with FlushContentAndNotify, and
nsDocument::FlushPendingNotifications only flushes the pres shell for
Flush_Layout.)

However, we're OK code wise, but we're not OK behavior-wise.  What
happens if the iframe on the inside has media queries that depend on its
dimensions?  We *would* have an infinite loop if you were flushing the
document rather than the pres shell, which, arguably, you should be
doing.  Avoiding this really just means the nasty behavior is
reflow-to-reflow rather than a bad loop in one reflow.

+    if (subdocHeight != mLastSubdocumentHeight) {

You should document carefully, perhaps with assertions at the
appropriate times, that both subdocHeight and mLastSubdocumentHeight
must be -1 unless ObtainIntrinsicSizeFrame (at what time?) returns a
frame with treatAsReplaced false.  That's a somewhat tricky invariant
that's preventing this from causing lots more reflows than necessary.


I'm not sure if the changes in nsHTMLReflowState should apply to the SVG
case where GetCrossDocLayoutParentFrame returns true.  Though I'm not
sure if it matters.

Do the reftests really all fit within the height that reftests do
comparison over?  (Though I suppose the comparison of the scrollbar
itself probably tests most of what you need.)

As far as I can tell, you removed the only caller of
nsSVGOuterSVGFrame::EmbeddedByReference that passes aEmbeddingFrame as
non-null, so you may as well remove the parameter from the function, and
the code that handles it.  Likewise, you also removed the only caller of
DependsOnIntrinsicSize.

That said, I'm having trouble understanding what codepath causes the
parent to reflow in the case where the code you're removing would have
caused it to.  Could you explain?  (It seems like we'd be going through
a different reflow path on the inside than ResizeReflow which seems to
be the only caller of NotifySubdocumentDidResizeReflow, and the code in
nsSubDocumentFrame::ReflowFinished only does anything in the
non-treatAsReplaced case.)


review- because I think we need to figure out what the invariants are as
far as passing flushes between parents and children, and because I think
we need to figure out what *should* happen with media queries here.
(One possible answer is that media queries inside seamless iframes refer
to the dimensions of the parent.  That would be pretty simple to do,
though perhaps harder to spec.)

I'd also like to understand how we're handling the dynamic width and
height changes on the inner SVG document.
Comment 79 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-11 11:07:46 PDT
(In reply to comment #78)
> caused it to.  Could you explain?  (It seems like we'd be going through
> a different reflow path on the inside than ResizeReflow which seems to
> be the only caller of NotifySubdocumentDidResizeReflow, and the code in
> nsSubDocumentFrame::ReflowFinished only does anything in the
> non-treatAsReplaced case.)

... and the latter wouldn't be sufficient anyway because it only handles height changes, not width-only changes.
Comment 80 David Ascher (:davida) 2008-09-26 10:25:48 PDT
Adding status whiteboard so the Thunderbird 3 folks can track core bugs we need.
Comment 81 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-10-20 06:37:06 PDT
(In reply to comment #78)
> (From update of attachment 335644 [details] [diff] [review])
> +   * document (e.g. <object> containing SVG or a "seamless" iframe) then
> 
> The code only checks the latter case.  (Updating just the comment seems
> ok.)

No, it returns a frame for the former case too. nsSubDocumentFrame::LayoutDependsOnSubdocument calls nsSubDocumentFrame::ObtainIntrinsicSizeFrame which checks for the SVG case.

> And in fact PresShell::ResizeReflow depends on the resulting frame being
> an nsIFrameFrame.  Given that callers assume that, you should probably
> document that in the header.

OK.

> +  if (!f->GetParent() || checkAncestorDocument) {
> 
> Maybe it's worth commenting on why this condition is OK -- in
> particular, that you need to pass aIntrinsicDirty up even if you broke
> out of the second loop due to wasDirty, because aIntrinsicDirty could be
> different.  However, you don't need this if aIntrinsicDirty == aResize
> (the weakest possibility), and that's the exact condition under which
> you skipped the loop in which checkAncestorDocument was set.  (Of
> course, it almost seems like unnecessary complexity.)

I'll see if I can simplify that.

> In nsSubDocumentFrame::ObtainIntrinsicSizeFrame, I tend to think that
> the checks for SVG and for _moz_seamless should be the other way around;
> the SVG behavior seems more powerful to me, so I think _moz_seamless
> shouldn't weaken it.  I *think* -- I'm not sure I understand the
> _moz_seamless behavior well enough, yet, though.

They're mutually exclusive --- only <iframe> elements can be _moz_seamless, and only <object> elements get the SVG behaviour (<iframe>s containing SVG don't get it, and never have).

> I'm almost tempted to say ComputeNonreplacedIntrinsicSize should use
> aCBWidth where it currently uses aAvailableWidth, since the presence of
> aAvailableWidth itself is almost a quirk (used to make things narrower
> when next to floats).  Though perhaps it should stay as you have it now.
> 
> Next to the definition of mIgnoreSubdocResizeReflow, maybe add a comment
> saying it's to suppress recursion?  Or maybe call the variable
> mDoingSubdocResize?

mDoingSubdocResize sounds good.

> The FlushPendingNotifications call inside ReflowFinished scares me a
> bit, although I don't know of anything problematic that it would cause
> right *now*.  It means, however, that we have flushes that go both up
> and down the tree, which means we're not far from getting into a
> situation where we can get flushes recurring into themselves in some
> cases by going from parent to child to parent (or the other way around).
> 
> (We're ok because nsDocument::FlushPendingNotifications calls up to the
> parent for Flush_Style or higher, nsPresShell::FlushPendingNotifications
> flushes the document only with FlushContentAndNotify, and
> nsDocument::FlushPendingNotifications only flushes the pres shell for
> Flush_Layout.)

I'll have to think about this.

> However, we're OK code wise, but we're not OK behavior-wise.  What
> happens if the iframe on the inside has media queries that depend on its
> dimensions?  We *would* have an infinite loop if you were flushing the
> document rather than the pres shell, which, arguably, you should be
> doing.  Avoiding this really just means the nasty behavior is
> reflow-to-reflow rather than a bad loop in one reflow.

I guess that's a spec question. I suspect that the "right" answer is to have the media queries for a seamless iframe use the parent's viewport data.

> +    if (subdocHeight != mLastSubdocumentHeight) {
> 
> You should document carefully, perhaps with assertions at the
> appropriate times, that both subdocHeight and mLastSubdocumentHeight
> must be -1 unless ObtainIntrinsicSizeFrame (at what time?) returns a
> frame with treatAsReplaced false.  That's a somewhat tricky invariant
> that's preventing this from causing lots more reflows than necessary.

Yes, OK, maybe I can simplify it.

> I'm not sure if the changes in nsHTMLReflowState should apply to the SVG
> case where GetCrossDocLayoutParentFrame returns true.  Though I'm not
> sure if it matters.

I think they should apply actually.

> Do the reftests really all fit within the height that reftests do
> comparison over?  (Though I suppose the comparison of the scrollbar
> itself probably tests most of what you need.)

Yes, they do. That's why I used a table to break them up into separate columns.

> As far as I can tell, you removed the only caller of
> nsSVGOuterSVGFrame::EmbeddedByReference that passes aEmbeddingFrame as
> non-null, so you may as well remove the parameter from the function, and
> the code that handles it.  Likewise, you also removed the only caller of
> DependsOnIntrinsicSize.

OK.

> That said, I'm having trouble understanding what codepath causes the
> parent to reflow in the case where the code you're removing would have
> caused it to.  Could you explain?  (It seems like we'd be going through
> a different reflow path on the inside than ResizeReflow which seems to
> be the only caller of NotifySubdocumentDidResizeReflow, and the code in
> nsSubDocumentFrame::ReflowFinished only does anything in the
> non-treatAsReplaced case.)

PresShell::FrameNeedsReflow on the nsSVGOuterSVGFrame should be marking the <object>'s nsSubDocumentFrame dirty.

> review- because I think we need to figure out what the invariants are as
> far as passing flushes between parents and children, and because I think
> we need to figure out what *should* happen with media queries here.
> (One possible answer is that media queries inside seamless iframes refer
> to the dimensions of the parent.  That would be pretty simple to do,
> though perhaps harder to spec.)

Er yeah, I agree :-). I don't know how hard it would be to spec.

I'll think more about the flushing issue when I'm more awake.
Comment 82 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-10-20 20:13:19 PDT
The flush ordering issue is a pain. I think if someone does a layout flush on the seamless child document, we'll do a layout flush on the parent which will trigger a layout flush on the child, then we'll return back out and do a layout flush on the child again. But I'm not sure what we can do about this other than rely on the fact that consecutive layout flushes without an intervening style or size change are idempotent and the subsequent flushes are O(1).
Comment 83 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-11-03 19:13:59 PST
I talked to dmose and we think he can work around the lack of this for Thunderbird 3, so cutting from 1.9.1.
Comment 84 Ben Bucksch (:BenB) 2009-12-14 14:59:48 PST
roc, a year passed since then. Is it possible to get this in now?
Comment 85 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-12-14 16:03:47 PST
No, we're swamped with other stuff.
Comment 86 Blake Winton (:bwinton) (:☕️) (PTO 'til London. Find me there for quick answers!) 2010-08-04 11:08:51 PDT
I know votes don't count for much, but of the four or five Thunderbird extensions I've started to write, I've wanted this behaviour twice.  Sure, twice isn't that much, but proportionally it's pretty big.  (I know you're swamped, so I don't expect to see it soon, but it would make my life much better.)

Thanks,
Blake.
Comment 87 Jonathan Protzenko [:protz] 2010-11-04 07:39:14 PDT
I'm doing a round of Roc-2.0-status-pings ;-). So Roc, is there any chance the latest improvements in the 2.0 cycle will make this easier to implement? Thanks! :)
Comment 88 David Ascher (:davida) 2010-11-16 09:31:41 PST
I've also wanted this kind of capability to do stuff like what people do in mobile phones w/ "mini browsers", where the navbar is a fixed height atop an iframe, with the parent iframe the one that does scrolling.  Right now there's no way to do that w/ gecko iframes w/o nasty adjusting of size based on measured content.  

Apparently "seamless" iframes is what we want?
Comment 90 Jonathan Protzenko [:protz] 2011-08-17 13:15:49 PDT
Created attachment 553880 [details] [diff] [review]
Updated patch

Here's an updated patch for mozilla-central. That was at times non-trivial to rebase, because files had been renamed, functions renamed, parts of the code rewritten, but I'm fairly confident this is in the spirit of roc's latest patch. Roc, could you possibly skim over this and maybe give more directions as to how to implement what you suggested in comment #82? I've left out the SVG parts of the patch for now, as I'd like to focus solely on the iframe issue. The reftests are included and pass (yay).

Also, I've tried to add an extra check for the case where the outer document is chrome and the seamless iframe is content, but:
- I don't know how to test it (can I use a chrome:// url in a reftest?),
- I'm not even sure I'm doing this right.
I'd love if you could verify that part :-).
Comment 91 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-17 17:53:27 PDT
Thanks for picking this up!!!

I think the reftest.js should not be needed. There is a way to do cross-origin reftests now, I think.

PR_MIN/MAX needs to change to NS_MIN/MAX.

We probably don't need to do anything about comment #82. We do need to address dbaron's comments though.

I'm not sure why the Subsumes check that I had doesn't do what you want. The extra code you added to IsCompatibleOriginSubdoc shouldn't be necessary.
Comment 92 Jonathan Protzenko [:protz] 2011-08-18 13:43:43 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #91)
> Thanks for picking this up!!!
> 
> I think the reftest.js should not be needed. There is a way to do
> cross-origin reftests now, I think.
I couldn't find it, so any pointers would be much appreciated :-).
> 
> PR_MIN/MAX needs to change to NS_MIN/MAX.
The function that uses these isn't referenced anywhere, did you have anything specific in mind when you wrote it, or is it a leftover of a previous version of the patch?
> 
> We probably don't need to do anything about comment #82. We do need to
> address dbaron's comments though.
Sure, I'm trying to address these right now!
> 
> I'm not sure why the Subsumes check that I had doesn't do what you want. The
> extra code you added to IsCompatibleOriginSubdoc shouldn't be necessary.
From the name I thought it didn't do what I wanted, but it actually does. I think I misread some previous comment.

My problem is the following:
1. my outer document is an HTML file, and is chrome,
2. I want a seamless iframe in it that has type="content" (contains potentially malicious content, since it's an email),
3. I want to access the iframe's docShell.

For 2., I don't know how HTML iframes work, so I'm not sure having a seamless html iframe inside a chrome html document would result in the iframe treated as content (any explanations welcome).

For 3., there's no docShell attribute on an html iframe. I may need to QI it to something more specific, but I don't know how to do that (idem).

Right now, I'm using a xul:iframe that satisfies both 2. and 3., but that patch doesn't help at all in the case of xul:iframes, if I understand things correctly...
Comment 93 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-18 18:43:12 PDT
(In reply to Jonathan Protzenko [:protz] from comment #92)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #91)
> > I think the reftest.js should not be needed. There is a way to do
> > cross-origin reftests now, I think.
> I couldn't find it, so any pointers would be much appreciated :-).

Neither can I! But I know you can do cross-origin tests with mochitest (see content/media/test/test_access_control.html for example), and do reftest-like things using WindowSnapshot.js, so you can test that way without modifying reftests.

> > PR_MIN/MAX needs to change to NS_MIN/MAX.
> The function that uses these isn't referenced anywhere, did you have
> anything specific in mind when you wrote it, or is it a leftover of a
> previous version of the patch?

Probably a leftover.

> > We probably don't need to do anything about comment #82. We do need to
> > address dbaron's comments though.
> Sure, I'm trying to address these right now!

Excellent!!

> > I'm not sure why the Subsumes check that I had doesn't do what you want. The
> > extra code you added to IsCompatibleOriginSubdoc shouldn't be necessary.
> From the name I thought it didn't do what I wanted, but it actually does. I
> think I misread some previous comment.

Great :-).

> My problem is the following:
> 1. my outer document is an HTML file, and is chrome,
> 2. I want a seamless iframe in it that has type="content" (contains
> potentially malicious content, since it's an email),
> 3. I want to access the iframe's docShell.
> 
> For 2., I don't know how HTML iframes work, so I'm not sure having a
> seamless html iframe inside a chrome html document would result in the
> iframe treated as content (any explanations welcome).
> 
> For 3., there's no docShell attribute on an html iframe. I may need to QI it
> to something more specific, but I don't know how to do that (idem).
> 
> Right now, I'm using a xul:iframe that satisfies both 2. and 3., but that
> patch doesn't help at all in the case of xul:iframes, if I understand things
> correctly...

I believe it should affect XUL <iframe>s. Does it not?
Comment 94 Jonathan Protzenko [:protz] 2011-08-18 19:59:40 PDT
Created attachment 554290 [details] [diff] [review]
Updated updated patch

Here's an updated patch that should address the review comments. I'm completely new to layout, so bear with me :-).

(In reply to David Baron [:dbaron] from comment #78)
> Comment on attachment 335644 [details] [diff] [review]
> completely updated patch
> 
> +   * document (e.g. <object> containing SVG or a "seamless" iframe) then
> 
> The code only checks the latter case.  (Updating just the comment seems
> ok.)
Comment updated.
> 
> And in fact PresShell::ResizeReflow depends on the resulting frame being
> an nsIFrameFrame.  Given that callers assume that, you should probably
> document that in the header.
I made that clearer in the header.
> 
> +  if (!f->GetParent() || checkAncestorDocument) {
> 
> Maybe it's worth commenting on why this condition is OK -- in
> particular, that you need to pass aIntrinsicDirty up even if you broke
> out of the second loop due to wasDirty, because aIntrinsicDirty could be
> different.  However, you don't need this if aIntrinsicDirty == aResize
> (the weakest possibility), and that's the exact condition under which
> you skipped the loop in which checkAncestorDocument was set.  (Of
> course, it almost seems like unnecessary complexity.)
I tried to simplify this, but I didn't manage to do so without breaking the logic. My former patch was wrong, and did this check in the for (;;) loop instead of after we've exited the loop. The new logic should be pretty similar to the old one, except for a few extra comments and a test that's been moved to the end of the function to make it a tiny bit clearer (imho).
> 
> 
> Up to nsFrameFrame.cpp.
Now nsSubDocumentFrame.cpp
> 
> In nsSubDocumentFrame::ObtainIntrinsicSizeFrame, I tend to think that
> the checks for SVG and for _moz_seamless should be the other way around;
> the SVG behavior seems more powerful to me, so I think _moz_seamless
> shouldn't weaken it.  I *think* -- I'm not sure I understand the
> _moz_seamless behavior well enough, yet, though.
This has been answered by roc.
> 
> I'm almost tempted to say ComputeNonreplacedIntrinsicSize should use
> aCBWidth where it currently uses aAvailableWidth, since the presence of
> aAvailableWidth itself is almost a quirk (used to make things narrower
> when next to floats).  Though perhaps it should stay as you have it now.
I didn't touch as I'm in no position to judge whether aCBWidth is better than aAvailableWidth. I'd vote for "we keep it as is".
> 
> Next to the definition of mIgnoreSubdocResizeReflow, maybe add a comment
> saying it's to suppress recursion?  Or maybe call the variable
> mDoingSubdocResize?
Did both.
> 
> 
> The FlushPendingNotifications call inside ReflowFinished scares me a
> bit, although I don't know of anything problematic that it would cause
> right *now*.  It means, however, that we have flushes that go both up
> and down the tree, which means we're not far from getting into a
> situation where we can get flushes recurring into themselves in some
> cases by going from parent to child to parent (or the other way around).
Does the "now" still hold?

(...)
> We *would* have an infinite loop if you were flushing the
> document rather than the pres shell, which, arguably, you should be
> doing.
I didn't change this part, should I?
> Avoiding this really just means the nasty behavior is
> reflow-to-reflow rather than a bad loop in one reflow.
> 
> +    if (subdocHeight != mLastSubdocumentHeight) {
> 
> You should document carefully, perhaps with assertions at the
> appropriate times, that both subdocHeight and mLastSubdocumentHeight
> must be -1 unless ObtainIntrinsicSizeFrame (at what time?) returns a
> frame with treatAsReplaced false.  That's a somewhat tricky invariant
> that's preventing this from causing lots more reflows than necessary.
I added a relevant assertion.
> 
> Do the reftests really all fit within the height that reftests do
> comparison over?  (Though I suppose the comparison of the scrollbar
> itself probably tests most of what you need.)
AFAICT, yes.
> 
> review- because I think we need to figure out what the invariants are as
> far as passing flushes between parents and children, and because I think
> we need to figure out what *should* happen with media queries here.
> (One possible answer is that media queries inside seamless iframes refer
> to the dimensions of the parent.  That would be pretty simple to do,
> though perhaps harder to spec.)
I implemented the "media queries inside seamless iframes should refer to the dimensions of the parent" bit. Is that enough to clear all concerns of infinite reflow loops, or is there more work needed? I didn't change anything for color, color index, and other media queries which do not depend on the size, as I believe this is the same inside the seamless iframe and outside of it. I also added an extra test for that.

I left out the SVG bits out, in the hope that this will make the patch simpler to review and to land.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #93)
> 
> Neither can I! But I know you can do cross-origin tests with mochitest (see
> content/media/test/test_access_control.html for example), and do
> reftest-like things using WindowSnapshot.js, so you can test that way
> without modifying reftests.
I'd rather not use a mochitest for that. MDN explicitly states that mochitest is overkill for that purpose, and, I'm quoting, “Try to avoid Mochitest” :-). On a personal note, I have the impression that reftest keeps my compile/test cycle faster than with mochitest.
> 
> > 
> > For 2., I don't know how HTML iframes work, so I'm not sure having a
> > seamless html iframe inside a chrome html document would result in the
> > iframe treated as content (any explanations welcome).
NeilAway said this was not possible :(.
> > 
> > For 3., there's no docShell attribute on an html iframe. I may need to QI it
> > to something more specific, but I don't know how to do that (idem).
khuey gave me some incantations based on QI that allow me to get the docshell.
> I believe it should affect XUL <iframe>s. Does it not?
I tried a chrome:// html page with an html:iframe pointing to google.com, the iframe was properly seamless. I replaced

  <div 
    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
    <iframe src="http://www.google.com" moz-seamless type="content">
    </iframe>

with

  <div 
    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
    <xul:iframe src="http://www.google.com" moz-seamless type="content">
    </xul:iframe>

and the xul:iframe was not visible because it had height 0. Am I missing something? Is the code different for HTML iframes and xul:iframes?
Comment 95 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-18 20:02:52 PDT
Comment on attachment 554290 [details] [diff] [review]
Updated updated patch

Probably more useful for roc to look at this first.
Comment 96 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-18 21:17:10 PDT
(In reply to Jonathan Protzenko [:protz] from comment #94)
> I tried a chrome:// html page with an html:iframe pointing to google.com,
> the iframe was properly seamless. I replaced
> 
>   <div 
>    
> xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>     <iframe src="http://www.google.com" moz-seamless type="content">
>     </iframe>
> 
> with
> 
>   <div 
>    
> xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>     <xul:iframe src="http://www.google.com" moz-seamless type="content">
>     </xul:iframe>
> 
> and the xul:iframe was not visible because it had height 0. Am I missing
> something? Is the code different for HTML iframes and xul:iframes?

Odd, I don't know why it should behave differently. There's an XBL binding but that should be OK. You probably will have to debug this.
Comment 97 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-18 21:18:27 PDT
If you really want to add cross-origin support to reftests, then you should do that as a separate patch. I'd just do the mochitest though. Sure, use reftests instead of mochitests if you can, but here you can't :-).
Comment 98 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-18 21:20:50 PDT
Oh hmm, does this fail for XUL IFRAMEs?
  nsCOMPtr<nsIDOMHTMLIFrameElement> iframe = do_QueryInterface(content);
?

I bet it does :-(. I think you should probably just eliminate that QI and test.
Comment 99 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-18 21:35:26 PDT
Comment on attachment 554290 [details] [diff] [review]
Updated updated patch

Review of attachment 554290 [details] [diff] [review]:
-----------------------------------------------------------------

The rest of it looks OK, although I wrote most of it so that isn't saying much :-)

::: layout/style/nsMediaFeatures.cpp
@@ +78,5 @@
>  };
>  #endif
>  
> +static nsPresContext*
> +GetRootPresContext_(nsPresContext* aPresContext) {

Call this GetSeamlessOuterPresContext?

@@ +99,5 @@
> +    nsPresContext* p = GetRootPresContext_(aPresContext);
> +    if (p)
> +        return GetRootPresContext(p);
> +    else
> +        return aPresContext;

Drop "else"
Comment 100 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-18 22:32:11 PDT
> Oh hmm, does this fail for XUL IFRAMEs?

Yes.  Why the heck is that QI there anyway?
Comment 101 Jonathan Protzenko [:protz] 2011-08-18 22:37:02 PDT
  if (content->Tag() == nsGkAtoms::iframe &&
      (content->IsHTML() || content->IsXUL()) &&
      content->HasAttr(kNameSpaceID_None, nsGkAtoms::moz_seamless)) {

I was thinking of using this instead.
Comment 102 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-18 23:21:07 PDT
I don't think we need that. We shouldn't need to check the element, because we won't create nsSubdocumentFrames for random elements.
Comment 103 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-18 23:34:21 PDT
What is that test actually trying to accomplish?  If you have a subdocument frame, that means that you have one of <html:frame>, <html:iframe>, <html:object> with a document inside, <xul:iframe>, <xul:browser>, <xul:editor>, or something that uses XBL to pretend to be one of those.

Is the point to only support moz_seamless on some subset of those?  If so, which ones?
Comment 104 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-19 01:24:06 PDT
I think we may as well support moz_seamless on all of them if that's helpful. But I could be wrong.

For HTML5 "seamless" we will need to avoid supporting it for <frame> and <object>.
Comment 105 Jonathan Protzenko [:protz] 2011-08-19 09:48:12 PDT
My reasoning was the following:
- we want this to be, in the end, html5 "seamless",
- html5 seamless is defined only for iframes, not object, not frame, not embed, not applet (for the latter I couldn't find a mention of it in the whatwg spec, but I don't think we want that either),
- I thought it was natural to have it only for xul iframes, then.

I'll change this to "an html iframe or any xul element that ended up being a nsSubDocument and has the attribute".
Comment 106 Jonathan Protzenko [:protz] 2011-08-19 15:43:44 PDT
Created attachment 554566 [details] [diff] [review]
Updated patch v3

This is an updated patch. New features compared to Roc's patch:
- Media queries in seamless iframes now use the outer PresShell.
- Various updates to make it work on current revisions of mozilla-central. The logic in nsPresShell::FrameNeedsReflow had changed a little bit, so the patch was refreshed.
- Spec: only HTML iframes can be seamless; any XUL nsSubDocument can be.
- There's a mochitest for XUL iframes being properly seamless.
- There's a mochitest for making sure a chrome page can host a seamless content iframe pointing to a remote domain.
- The reftests have been converted to mochitests (and use WindowSnapshot.js) to make it easier to work with cross-domain tests.

As roc said, there's not much in there that he didn't write in the first place. If I understood correctly, the media queries should guarantee that we don't end up endlessly going up and down in a reflow loop. If there's more work needed to make sure that doesn't happen, I'll gladly implement it, but I may need guidance, probably in a more detailed fashion than if it were for roc :).
Comment 107 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-19 19:51:31 PDT
Instead of this:

> +  if ((content->IsHTML() && content->Tag() == nsGkAtoms::iframe || content->IsXUL())

  if (content->IsHTML(nsGkAtoms::iframe) || content->IsXUL()) {

would be more readable.

On a meta-review note, please add "showfunc = true" to the [diff] section of your .hgrc.  It produces patches that are somewhat easier to review.
Comment 108 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-20 05:12:56 PDT
(In reply to Jonathan Protzenko [:protz] from comment #105)
> My reasoning was the following:
> - we want this to be, in the end, html5 "seamless",
> - html5 seamless is defined only for iframes, not object, not frame, not
> embed, not applet (for the latter I couldn't find a mention of it in the
> whatwg spec, but I don't think we want that either),
> - I thought it was natural to have it only for xul iframes, then.
> 
> I'll change this to "an html iframe or any xul element that ended up being a
> nsSubDocument and has the attribute".

That makes complete sense!
Comment 109 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-08-22 07:54:03 PDT
FWIW, this looks like a sizeable review, and I'm on vacation this week (though with plenty of internet access) and might not get to it for a little bit.

It would help, though, if you could briefly summarize what set of features this patch is intending to implement, and how (if at all) they relate to any of the related features in HTML5.
Comment 110 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-22 16:05:48 PDT
The goals here are quite limited:
-- Give an <iframe seamless> an intrinsic preferred width and intrinsic minimum width that are based on the content (the intrinsic preferred width and intrinsic minimum width of the root element, plus margins, basically)
-- Give an <iframe seamless> an auto-width as if it's not a replaced element (i.e., max(min-width, min(pref-width, available width)))
-- Give an <iframe seamless> an auto-height as if it's not a replaced element (i.e., the height of its contents when reflowed to its computed width)
Comment 111 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-22 16:09:16 PDT
This is basically items 4-6 of HTML5's "seamless":
http://www.whatwg.org/specs/web-apps/current-work/complete.html#attr-iframe-seamless
Comment 112 Jonathan Protzenko [:protz] 2011-10-05 05:09:57 PDT
David, is there anything I can do to make the review easier for you? If the patch needs rebasing, I can do that if that makes it easier for you to review it.
Comment 113 Jonathan Protzenko [:protz] 2011-11-02 15:02:18 PDT
David, if you're too busy with other stuff, is there someone else I can ask for review? :)
Comment 114 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-11-30 22:25:16 PST
Comment on attachment 554566 [details] [diff] [review]
Updated patch v3

nsLayoutUtils.h:

>+   * Get the parent of aFrame. If aFrame is the root frame for a document,
>+   * and the document has a parent document in the same view hierarchy,
>+   * and the layout of the parent document depends on the embedded
>+   * document (e.g. a "seamless" iframe) then we try to return the
>+   * nsSubDocumentFrame in the parent document.

There's a bunch in this comment that's superfluous.  I think you should
boil it down to:

    Get the parent of aFrame.  However, if aFrame is the root frame for
    document, and the document's container depends on the embedded
    document (i.e., a "seamless" iframe) then traverse into the parent
    document and return that iframe.

If any of the other conditions that you mentioned (i.e., seamless
iframe not in same view hierarchy or "try to" not succeeding) actually
matter then we have a problem that needs to be fixed.

In PresShell::FrameNeedsReflow, in the case where you propagate the need
to reflow up to an ancestor, I think perhaps you should bail rather than
calling MaybeScheduleReflow.  It's both faster (only one reflow gets
scheduled) and avoids having to worry about the complication of what
happens if the inner document gets reflowed first.  (I could certainly
imagine something like an onresize handler on the inside causing an
infinite loop if we did that.)  But it does add one complication,
which is that in the code in nsSubDocumentFrame::AttributeChanged, you'd
need to handle the case where moz-seamless is removed by enqueueing the
needed reflow (since you can't guarantee that the change will actually
cause the iframe to resize).

In nsPresShell.cpp I'm a little suspicious of only calling
subdocFrame->NotifySubdocumentDidResizeReflow() in the one place you do
rather than inside of DidDoReflow (which catches a bunch of other places
too).  Have you checked that there's nothing that can cause a reflow in
a subdocument without going through PresShell::FrameNeedsReflow?

I'd prefer to remove the word "Resize" from
NotifySubdocumentDidResizeReflow.  Resize reflow is a particular concept
that we used to have but largely doesn't exist anymore.  Or if it's more
specific (see previous comment) use some other name, like
"DelayedResizeReflow".

+static PRBool
+Implies (PRBool a, PRBool b) {
+  return (!a || a && b);
+}

"static PRBool" -> "static inline bool"
"return (!a || a && b);" -> "return !a || b;"

And please s/PRBool/bool/g;s/PR_TRUE/true/g;s/PR_FALSE/false/g throughout.

In nsSubDocumentFrame::AttributeChanged you should check that the tag
name is iframe or that it's XUL (you might want to consolidate that into
a function for whether the seamless attribute applies, since it's also
used in ObtainIntrinsicSizeFrame).

In nsSubDocumentFrame::ComputeSize:
>     return nsLayoutUtils::ComputeSizeWithIntrinsicDimensions(
>-                            aRenderingContext, this,
>-                            subDocRoot->GetIntrinsicSize(),
>-                            subDocRoot->GetIntrinsicRatio(),
>-                            aCBSize, aMargin, aBorder, aPadding);
>+                                  aRenderingContext, this,
>+                                  subDocRoot->GetIntrinsicSize(),
>+                                  subDocRoot->GetIntrinsicRatio(),
>+                                  aCBSize, aMargin, aBorder, aPadding);
>   }

This reindentation is gratuitous.  Leave it as it was.


nsSubDocumentFrame::ObtainIntrinsicSizeFrame should have a default value
for the parameter; then you won't have to change existing callers to add
"nsnull" (which doesn't make a whole lot of sense when you look at the
caller).

I'm also inclined to suggest that its out param should be an enum rather
than a boolean, with values { INTRINSIC_SIZE_REPLACED,
INTRINSIC_SIZE_SEAMLESS }.  Maybe even a third value for the case
where it returns null.

Please s/PR_MAX/NS_MAX/g.  also MIN, if there are any.

Please add a comment to nsSubDocumentFrame::ReflowFinished noting that
the handling of mDoingSubdocResize is not exception-safe, but that it's
also hard to use AutoRestore or AutoToggle (bug 518756).


nsSubDocumentFrame::ReflowFinished

  I'm really not comfortable with flushing in the middle of reflow.  Maybe
  that's just because I don't remember the rules well enough, though.

  This reflow setup in general seems unnecessarily roundabout and
  inefficient.  Why can't we just reflow the child when we need to, and
  add a mechanism to avoid the resize suppression that might happen?
  That seems likely to be simpler than the current ReflowFinished
  behavior where we fix things up after reflow by reflowing again.

ObtainIntrinsicSizeFrame again:

>+  if ((content->IsHTML() && content->Tag() == nsGkAtoms::iframe || content->IsXUL())

Please do ((a && b) || c) rather than relying on operator precedence.


nsMediaFeatures.cpp:

  Opening { of a function should be on its own line (both new functions).

  >+// is recursive (in case of nested seamless iframes, heh, you never know).

  Drop the ", heh, you never know".  Interactions of features are
  quite common.

  We already have a concept called a root pres context, so
  GetRootPresContext should be called something else, since it's
  different.

The patch should have a commit message included in it.


So now I'm going to look at this as an implementation of **part** of the
HTML5 seamless attribute.  Since we don't implement sandox, it seems
that the following requirement from
http://www.whatwg.org/specs/web-apps/current-work/#attr-iframe-seamless
is the only part of the initial paragraph that applies:
  # while either the browsing context's active document has the same
  # origin as the iframe element's document, or the browsing context's
  # active document's address has the same origin as the iframe
  # element's document, or the browsing context's active document is an
  # iframe srcdoc document

Then below, in the "the following requirements apply", I'm presuming
that this patch is attempting to implement only the three "In visual
media," items listed there (and not the rest of the items, though they
actually do apply):

  # In visual media, in a CSS-supporting user agent: the user agent
  # should set the intrinsic width of the iframe to the width that the
  # element would have if it was a non-replaced block-level element with
  # 'width: auto'.
  #
  # In visual media, in a CSS-supporting user agent: the user agent
  # should set the intrinsic height of the iframe to the height of the
  # bounding box around the content rendered in the iframe at its
  # current width (as given in the previous bullet point), as it would
  # be if the scrolling position was such that the top of the viewport
  # for the content rendered in the iframe was aligned with the origin
  # of that content's canvas.
  #
  # In visual media, in a CSS-supporting user agent: the user agent must
  # force the height of the initial containing block of the active
  # document of the nested browsing context of the iframe to zero.
  #
  #   Note: This is intended to get around the otherwise circular
  #   dependency of percentage dimensions that depend on the height of
  #   the containing block, thus affecting the height of the document's
  #   bounding box, thus affecting the height of the viewport, thus
  #   affecting the size of the initial containing block.


I'm worried about a few things here:

 * when the iframe document is navigated, what causes us to reevaluate
   whether it should be seamless (and redo the security checks)?

 * Do we do the height calculation correctly?  It's not even clear to me
   what the "bounding box around the content" means.  Do we include
   floats and absolutely positioned elements?  (I think we definitely
   should.)  Do we include overflow?  (I'm less sure here.)  What about
   fixed positioning?

Someone also needs to send a comment on the spec that it needs to say
that media queries in the seamless iframe evaluate as for the outside
presentation (as this implements, and needs to).


I think this is pretty close, but I'd like to look at another revision
that addresses these comments, so marking review-.  Sorry for taking so
long to get to this; the next time around should be much faster.

I also haven't yet reviewed the tests, though I feel like I should.  To
do so, it would probably help to have a summary of what they test.
Comment 115 Hixie (not reading bugmail) 2011-12-01 15:03:36 PST
Good point about the media queries. I've noted it:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=15033
Comment 116 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-01 16:36:36 PST
I also filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=15035 on "the height of the bounding box around the content" being imprecise.
Comment 117 Jonathan Protzenko [:protz] 2011-12-06 01:11:06 PST
Thanks for the review! There's quite a bit to address, and I must confess the details have been fading out somehow, but I'll try to do my best to send a new patch shortly.
Comment 118 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-01-25 14:14:59 PST
[secr:curtisk] talk to dbaron about this
Comment 119 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-03-13 07:33:15 PDT
:dbaron or :roc what do we need or want to do here security wise?
Comment 120 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-19 00:49:26 PDT
The obvious concern is about information leaks from reading the size of iframes. We're trying to mitigate that by only enabling this feature for same-origin iframes.
Comment 121 Henri Sivonen (:hsivonen) 2012-03-19 01:01:49 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #120)
> The obvious concern is about information leaks from reading the size of
> iframes. We're trying to mitigate that by only enabling this feature for
> same-origin iframes.

Aren't there legitimate different-origin use cases? E.g. blog comments outsourced to a comment service.

If it's same-origin by default, I think we should support the crossorigin attribute and CORS for relaxing the restriction.
Comment 122 Ben Bucksch (:BenB) 2012-03-19 01:10:39 PDT
> only enabling this feature for same-origin iframes.

We definitely also need this for embedding untrusted content in privileged chrome XUL. In fact, that's the main usecase.

We should discuss your concerns and possible mitigation in the meeting, because I think a lot of the usecases for this feature are indeed cross-origin. If it was the same server and security context, the server could just embed the text directly. This feature here could e.g. also be used for separating HTML emails from HTML webmail apps, and similar cases in HTML apps.
Comment 123 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-19 02:27:41 PDT
(In reply to Henri Sivonen (:hsivonen) from comment #121)
> Aren't there legitimate different-origin use cases? E.g. blog comments
> outsourced to a comment service.

Sure. Security concerns getting in the way of valid use-cases is nothing new :-).

> If it's same-origin by default, I think we should support the crossorigin
> attribute and CORS for relaxing the restriction.

We could, and probably should, but this would need to be specced as an extension to the HTML5 "seamless" feature, which currently requires same-origin.

(In reply to Ben Bucksch (:BenB) from comment #122)
> We definitely also need this for embedding untrusted content in privileged
> chrome XUL. In fact, that's the main usecase.

There's no problem allowing "seamless" for cross-origin content if the outer frame is trusted (although we have to be careful not to leak privileges into the inner frame).
Comment 124 Joshua Cranmer [:jcranmer] 2012-06-04 16:39:21 PDT
I've been told by protz that he lacks the time to work on this any further. Is there anyone willing to help drive the patch into the tree?
Comment 125 Joshua Cranmer [:jcranmer] 2012-06-09 19:37:09 PDT
Created attachment 631712 [details] [diff] [review]
Derotted patch

I've updated the last patch and made sure it applies to tip-ish trunk without errors. I've also fixed the trivial stuff dbaron pointed out: PRBool issues, whitespace, default parameter, etc. Any of his comments that required understanding the code to be able to do or respond to I did not do since I have no clue about this part of the codebase.
Comment 126 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-09-26 04:53:49 PDT
CC'ing Chris Jones. This may help with one of the keyboard issue on Gaia.

The Gaia keyboard is a regular frame in-process (<iframe src=keyboard.html>) because it needs to communicate with the system application via postMessage to know about size changes.

It seems like seamless iframes will let us get rid of the postMessage pseudo protocol and the keyboard would be able to be a OOP mozbrowser (<iframe mozbrowser mozapp="manifest.webapp" src="keyboard.html">) frame that could be killed when it is unused and save some memory.

Nominating blocking-basecamp.
Comment 127 Jonas Sicking (:sicking) PTO Until July 5th 2012-09-26 10:47:05 PDT
Vivien: Isn't there an easier way to solve this? I'm concerned about trying to cram this in before next branch point which is happening in less than 2 weeks.

Roc, or anyone else from the layout team: Is there any chance we can have this landed before the next branch point?
Comment 128 Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) 2012-09-26 20:18:42 PDT
Vivien, I thought bug 757859 is the plan to get rid of postMessage pseudo protocol?
Comment 129 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-09-27 13:30:10 PDT
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #128)
> Vivien, I thought bug 757859 is the plan to get rid of postMessage pseudo
> protocol?

There where some issues with this approach as it was not working in some cases. I don't remember the exact details but Dale Harvey can explain better.
Comment 130 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-09-27 14:06:30 PDT
(In reply to Jonas Sicking (:sicking) from comment #127)
> Vivien: Isn't there an easier way to solve this? I'm concerned about trying
> to cram this in before next branch point which is happening in less than 2
> weeks.
> 

I just discussed with cjones on IRC. Seems like a in-process keyboard would be enough for this version. Removing blocking-basecamp?
Comment 131 Thomas D. (currently busy elsewhere; needinfo?me) 2012-10-10 12:08:00 PDT
@Curtis Koenig [:curtisk]

It has been confirmed by a number of important Mozilla figures like :gerv and :bwinton that this 11-year-old bug is much-needed:
- work towards implementation of HTML5 seamless attribute for <iframe> (I understand from current summaries that this bug 80713 wants to do moz-seamless, and bug 631218 does the "real" seamless attribute)
- this bug blocks a number of bugs which are important for the development of Thunderbird, SeaMonkey and addons, as confirmed by a lot of relevant people including :bwinton (TB UX lead) (see Comment 86)

You opened Bug 748945 for the security review of this bug in April 2012.
sec-review?curtisk was requested mid-August 2012.

Do you need more information to do the sec-review?
Do you actually need an answer to all those questions of Bug 748945 before you will start the sec-review?
Are you still willing/able/assigned to do the sec-review for this bug?
If yes, can you provide an estimated target date for this sec-review?

Any help to push the never-ending saddening story of this bug towards completion would be much appreciated.

> On 2012-04-25, :curtisk created: Bug 748945 (security review of this bug 80713)
> On 2012-04-26, :curtisk set: Whiteboard:[sec-assigned:curtisk:748945]
> On 2012-08-15, :dveditz set: Flags: sec-review?(curtisk@mozilla.com)
> On 2012-08-17, :curtisk removed: Whiteboard:[sec-assigned:curtisk:748945]
Comment 132 Thomas D. (currently busy elsewhere; needinfo?me) 2012-12-21 07:00:02 PST
Curtis, could you reply to my comment 131?
Sorry, forgot to set the needinfo flag ;)
Comment 133 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-12-21 07:15:46 PST
(In reply to Thomas D. from comment #131)
> @Curtis Koenig [:curtisk]
> 
> It has been confirmed by a number of important Mozilla figures like :gerv
> and :bwinton that this 11-year-old bug is much-needed:
> - work towards implementation of HTML5 seamless attribute for <iframe> (I
> understand from current summaries that this bug 80713 wants to do
> moz-seamless, and bug 631218 does the "real" seamless attribute)
> - this bug blocks a number of bugs which are important for the development
> of Thunderbird, SeaMonkey and addons, as confirmed by a lot of relevant
> people including :bwinton (TB UX lead) (see Comment 86)
> 
> You opened Bug 748945 for the security review of this bug in April 2012.
> sec-review?curtisk was requested mid-August 2012.
> 
> Do you need more information to do the sec-review?

Yes

> Do you actually need an answer to all those questions of Bug 748945 before
> you will start the sec-review?

Yes all those questions need to be answered to proceed with a sec review.

> Are you still willing/able/assigned to do the sec-review for this bug?
> If yes, can you provide an estimated target date for this sec-review?

This will be a team sec-review not just me, hence why we need the questions answered especially the last one.

> Any help to push the never-ending saddening story of this bug towards
> completion would be much appreciated.
> 
> > On 2012-04-25, :curtisk created: Bug 748945 (security review of this bug 80713)
> > On 2012-04-26, :curtisk set: Whiteboard:[sec-assigned:curtisk:748945]
> > On 2012-08-15, :dveditz set: Flags: sec-review?(curtisk@mozilla.com)
> > On 2012-08-17, :curtisk removed: Whiteboard:[sec-assigned:curtisk:748945]
Comment 134 Herman van Rink 2013-02-14 02:54:25 PST
I've just opened an issue for the Conversations extension. And it seems that this bug is (part of) the cause.

https://github.com/protz/GMail-Conversation-View/issues/703 

Just adding it here for reference.
Comment 135 Thomas D. (currently busy elsewhere; needinfo?me) 2013-04-06 10:22:33 PDT
This bug is certainly worthy enough to deserve more attention.*

1) This bug needs a new owner - currently assigned to nobody.

-> Who has sufficient coding and code knowledge to finish this off?

2) The current patch of this bug needs attention (attachment 631712 [details] [diff] [review]). My impression is that large parts of the work required have already been done. Even formal issues of the previous patch have been addressed (Comment 125). What's left is to answer and address as required the bottom half of Comment 114:

> So now I'm going to look at this as an implementation of **part** of the
> HTML5 seamless attribute [snip]

-> Who has the right knowledge to look into these implementation details?

3) The security review required for this bug needs answers to the respective questions raised in Bug 748945 Comment 0. We should probably move on with that concurrently with finishing the patch, so that possibly arising security issues can be factored into the patch, and to speed up the process.

-> Who can answer some of the respective questions raised in Bug 748945 Comment 0? (I think answers probably need to be collated here or somewhere else inside or outside bmo before we hand them over formally to bug 748945).

*: Here's why moving this bug forward is a worthy cause:

1) This bug has been confirmed to be important by the powers that be, because it blocks a whole lot of other important bugs (see Comment 131)
2) This bug is very old (filed 2001) - dogfood?
3) This bug appears stalled (once again), unless we find the right people to move this forward as described above.
Comment 136 Tim Nguyen :ntim 2014-01-01 05:07:26 PST
Note that height:-moz-fit-content; and width:-moz-fit-content might be useful here.
Comment 137 Thomas D. (currently busy elsewhere; needinfo?me) 2014-06-23 04:17:33 PDT
Gerv, could you please assist to get this important bug moving again? Tia.

According to Comment 8, this was inspired by your blogpost:
http://weblogs.mozillazine.org/gerv/archives/007610.html

- filed 2001...
- 47 votes, 89 CC'ed, 136 comments
- currently blocking 9 other bugs, some of them with major design implications
- confirmed as important by relevant figures like
  - Gerv (see blogpost URL above)
  - Blake Winton (Comment 86)
  - David Ascher (Comment 62, comment 80, comment 88)
  - Joshua Cranmer (Comment 124 & updated patch)
  - Ben Bucksch (Comment 61, comment 122)

Current status of this bug:

- assigned to nobody
- has draft patch by :roc (unbitrotted by :jcranmer in attachment 631712 [details] [diff] [review])
- draft patch has outstanding review questions (Comment 114 lower half)
- security review has outstanding questions (Bug 748945 Comment 0)
- tentative summary of next steps to get started: see my Comment 135
Comment 138 Kevin Brosnan [:kbrosnan] 2014-06-23 12:05:44 PDT
Gerv is not an engineering manager, he is not going to be able to move this bug forward. If you wish to discuss the status of this bug please use https://lists.mozilla.org/listinfo/dev-platform or via nntp://news.mozilla.org list mozilla.dev.platform
Comment 139 Gervase Markham [:gerv] 2014-06-24 00:35:39 PDT
I think that progress could be made on that bug by getting the patch unbitrotted again and the security review restarted. Neither of those things necessarily requires an engineering manager.

The comment Curtis posted in the secreview bug are just his standard secreview question checklist. Those can be answered without too much difficulty.

What this bug does need, though, is someone (jcranmer?) who is willing to re-unbitrot it and steer it through the process. That's not me, I'm afraid. It hopefully shouldn't require too much time - roc did the hard part in working out how to do this and proving it worked - but it just needs the right skills.

Gerv
Comment 140 Jonathan Protzenko [:protz] 2014-06-24 01:35:38 PDT
I un-bitrotted this patch earlier on; if I recall correctly, some concerns were raised in comment #82 and earlier; I tried to address some of them (the easy ones) but properly addressing dbaron's review requires proficiency in the layout codebase which I, unfortunately, don't possess. According to comment #125, jcranmer's un-bitrotted patch still contains some issues that require fixing.

(I would say properly finishing the patch is a prerequisite before moving on to the security review.)

Thanks,

~ jonathan
Comment 141 Joshua Cranmer [:jcranmer] 2014-06-24 08:00:38 PDT
(In reply to Gervase Markham [:gerv] from comment #139)
> What this bug does need, though, is someone (jcranmer?) who is willing to
> re-unbitrot it and steer it through the process. That's not me, I'm afraid.
> It hopefully shouldn't require too much time - roc did the hard part in
> working out how to do this and proving it worked - but it just needs the
> right skills.

As I mentioned in comment 125, I'm not sufficiently well-acquainted with the code to make non-trivial changes to the patch. As protz says, this requires familiarity with the layout codebase, and I lack this knowledge.
Comment 142 Gervase Markham [:gerv] 2014-06-25 08:22:29 PDT
So it seems like those who want this bug pushed forward need to find a code champion.

Gerv
Comment 143 dotraix 2014-06-25 10:11:03 PDT
Does Mozilla do 'bug bounties'? (i.e. users put up a dollar amount that whoever completes the bug can claim)
Comment 144 Saurabh Anand [:sawrubh] 2014-06-25 10:23:23 PDT
https://www.bountysource.com/trackers/268848-mozilla might be what you're looking for.
Comment 145 Gervase Markham [:gerv] 2014-06-26 08:25:02 PDT
I'm not sure this is an issue solvable with money, as there are (sadly) only a small number of people with the understanding necessary to fix up the patch.

Gerv
Comment 146 Ben Bucksch (:BenB) 2014-06-26 11:26:22 PDT
At least not that kind of money. $50000 on one of the layout dev's personal bank account would get his attention, I guess :).

I can't emphasize how important this feature is. iframes are important security sandboxes, esp. in today's mix-and-match web.

I needed this in Thunderbird, to have the email headers scroll with the message body, but still keep the headers privileged (to show reliable security indicators, to access address book etc.), while keeping the message body untrusted. Every webmail app has the same problem. They try to solve it by server-side sanitizing, but that obviously goes only this far. We have other boxing proposals, but none is as strong as an iframe from a separate (!) domain to secure untrusted content from trusted (webapp) UI.

This is important for any webapp that wants to display untrusted data, but scroll trusted UI with it.

comment 120:
> The obvious concern is about information leaks from reading the size of iframes. We're trying to
> mitigate that by only enabling this feature for same-origin iframes.

We already discussed this above (comment 122), but the "information leak" here is minor compared to the importance of this feature for security protection of webapps against untrusted content. This can significantly *help* security much more, against serious XSS holes, than the damage that such a minor information leak can do. In terms of https://wiki.mozilla.org/Security_Severity_Ratings, this is sg:high vs. sg:low.

So, unless there are really major security concerns, I would vote for the patches to go in.
Comment 147 Ben Bucksch (:BenB) 2014-06-26 11:36:43 PDT
FWIW, IIRC, this was also important for the Firefox OS Gaia email app. They had to put untrusted content (!) in the Gaia UI context (!), after sanitizing the HTML of course, because they wanted that scrolling UI and couldn't find another way, because this particular feature is missing, and this bug wasn't moving forward. An iframe + sanitizing of course gives much better protection than sanitizing alone - same as on every webapp out there. So, this is relevant to - and endangering security on - Firefox OS, too. (Please correct/deny or confirm.)
Comment 148 David Ascher (:davida) 2014-06-26 15:01:39 PDT
Given Ben's last comment -- has anyone recently explored whether the approach taken by FFOS/Email (sanitizing HTML and displaying w/o iframe) couldn't work for TB?
Comment 149 Ben Bucksch (:BenB) 2014-06-26 16:51:18 PDT
David, I believe that approach is fundamentally unsafe. I'm all for sanitizing, in fact the HTML sanitizer in Thunderbird was my idea and implementation, but it cannot replace the protection that an iframe can give on the browser level. I would never ever take untrusted HTML, sanitize it, and stick it into chrome context.
The web is stock full of XSS holes. (Which is exactly why we need this feature!) But in Thunderbird, it's a full compromise of the local machine with all data and privileges, which is a lot more severe even. That's begging for disaster.
When it comes to security, and you have only one layer of protection, you very quickly find yourself out in the cold naked.
Comment 150 Andrew Sutherland [:asuth] 2014-06-27 07:44:04 PDT
(In reply to Ben Bucksch (:BenB) from comment #147)
> FWIW, IIRC, this was also important for the Firefox OS Gaia email app. They
> had to put untrusted content (!) in the Gaia UI context (!), after
> sanitizing the HTML of course, because they wanted that scrolling UI and
> couldn't find another way, because this particular feature is missing, and
> this bug wasn't moving forward. An iframe + sanitizing of course gives much
> better protection than sanitizing alone - same as on every webapp out there.
> So, this is relevant to - and endangering security on - Firefox OS, too.
> (Please correct/deny or confirm.)

This is incorrect-ish.  Seamless iframes are largely orthogonal to security for the Firefox OS Gaia email app, and entirely orthogonal for Thunderbird.  The Gaia email app sanitizes because it lacks the ability to create an nsIContentPolicy like Thunderbird in order to prevent remote resource access.  We create a sandboxed iframe in the same origin because being in the same origin lets us manipulate the DOM/listen to events, although we could avoid doing that by jumping through additional hoops.

Since we are in the same origin we are able to approximate a seamless iframe since the only changes to our DOM that occur are performed synchronously by us or as the result of image loads that we can listen for the load events on.

Since Thunderbird a) has a chrome context and is able to reach into the content DOM no matter its origin, and b) has an nsIContentPolicy, any security issues would be of Thunderbird's own creation in an attempt to avoid visual artifacts/glitchy behaviour.

Having said that, this is going to be a problem for the Gaia email app once we get pinch/zoom in our sub-iframes happening.  However, we've had very good communication with the layout and graphics/APZ teams and it is my understanding they are well aware of this.  This is at best a medium-term concern for the Gaia email app.
Comment 151 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2014-07-10 15:20:05 PDT
closing out the stale sec-review bug but leaving the flag for review on this bug. If when we get to a date / place where we can do a sec review please ping with a need-info and we'll come.

Note You need to log in before you can comment on or make changes to this bug.