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

0.9.1 and P2 since the block-bug is pretty common and pretty nasty.

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?

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.

Great, thanks. Moving to 0.9.2

The standards are silent on this issue. I don't know of a way to do it with our implementation as is.

by mandate, moving non-crashers and non-datalossers outward.

.

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.

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.

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).

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

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...)

> 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).

I think comment 13 is a security issue that we need to worry about.

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

(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.

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.

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

Yeah, that's possible, but I can't think of anything better.

(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.

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.

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.

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.

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.

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

roc: is there any chance of progress here? Are the security issues solved to your satisfaction? Gerv

I think we should push this in.

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.

(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.

(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.

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

If we do same-origin checks on documents, we should really use CheckSameOriginPrincipal, not CheckSameOriginURI....

We're assuming that authors don't rely on the height of an IFRAME being 150px, I take it?

Yes. Do you think that's an unreasonable assumption? Is the default different in other browsers? Gerv

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).

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.

Could we add a rule to html.css like: iframe:not([height]) { height: 100px; } (or whatever the default height currently is)

Sure, but why bother with the :not([height]) bit? If height is set to a number, it would override the ua.css rule anyway.

Even if it is specified as an attribute? I don't remember the cascade rules offhand.

Presentational attributes override non-important UA and user rules.

blocking-, renominate when there's consensus on web-compat and XSS issues.

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

(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 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.

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?

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.

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

(1) the spec says so (2) as I said above, I don't want this behavior to happen for the default value

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

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.

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

I'd suggest not -moz-auto. How about -moz-child-document instead? It's up to David really.

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

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.

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.

> 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.

Nominating for wanted1.9.1. From Thunderbird's POV, we don't care what the syntax is.

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!)

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).

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.

Since I wrote the original patch, jwatt implemented intrinsic sizing for <object> embedding SVG. So we just need to extend that somewhat.

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.

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?

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.

> 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.

Would doing the whole "batch inserts, not notifications" thing help? Not actually sure that it would given XBLs involvement.

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.

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.

(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.

In comment #73 when I wrote "message pane" I mean specifically the message *body* pane containing only the sandboxed message content.

[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.

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 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.

(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.

Adding status whiteboard so the Thunderbird 3 folks can track core bugs we need.

(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.

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).

I talked to dmose and we think he can work around the lack of this for Thunderbird 3, so cutting from 1.9.1.

roc, a year passed since then. Is it possible to get this in now?

No, we're swamped with other stuff.

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.

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! :)

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?

ref: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-seamless

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 :-).

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.

(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...

(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?

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 on attachment 554290 [details] [diff] [review] Updated updated patch Probably more useful for roc to look at this first.

(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.

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 :-).

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 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"

> Oh hmm, does this fail for XUL IFRAMEs? Yes. Why the heck is that QI there anyway?

if (content->Tag() == nsGkAtoms::iframe && (content->IsHTML() || content->IsXUL()) && content->HasAttr(kNameSpaceID_None, nsGkAtoms::moz_seamless)) { I was thinking of using this instead.

I don't think we need that. We shouldn't need to check the element, because we won't create nsSubdocumentFrames for random elements.

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?

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>.

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".

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 :).

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.

(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!

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.

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)

This is basically items 4-6 of HTML5's "seamless": http://www.whatwg.org/specs/web-apps/current-work/complete.html#attr-iframe-seamless

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.

David, if you're too busy with other stuff, is there someone else I can ask for review? :)

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.

Good point about the media queries. I've noted it: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15033

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.

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.

[secr:curtisk] talk to dbaron about this

:dbaron or :roc what do we need or want to do here security wise?

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.

(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.

> 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.

(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).

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?

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.

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.

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?

Vivien, I thought bug 757859 is the plan to get rid of postMessage pseudo protocol?

(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.

(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?

@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]

Curtis, could you reply to my comment 131? Sorry, forgot to set the needinfo flag ;)

(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]

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.

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.

Note that height:-moz-fit-content; and width:-moz-fit-content might be useful here.

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

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

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

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

(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.

So it seems like those who want this bug pushed forward need to find a code champion. Gerv

Does Mozilla do 'bug bounties'? (i.e. users put up a dollar amount that whoever completes the bug can claim)

https://www.bountysource.com/trackers/268848-mozilla might be what you're looking for.

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

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.

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.)

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?

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.

(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.

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.