Attaching OBJECT during printing doesn't work |
|||||||||
Issue descriptionAttaching a LayoutEmbeddedObject simply doesn't seem to work correctly if it happens during printing. See test (and test it with the legacy engine). This is very serious for LayoutNG, since we then always re-attach the entire document when entering printing (LayoutNG doesn't support printing, so we need to fall back to legacy layout, and rebuild everything). For LayoutNG this means that it's impossible to print anything that creates a LayoutEmbeddedObject, e.g. an OBJECT element with an SVG, or an OBJECT element with HTML content (aka "iframe"). This is the reason why printing/iframe-svg-in-object-print.html fails in LayoutNG. My observation is that HTMLFrameOwnerElement::SetEmbeddedContentView() is called with a nullptr argument during layout object tree reattachment (while tearing down the old tree) when entering printing, and nobody is ever calling it again with something non-null. The child frame *is* laid out when entering printing, though. It's just that it doesn't get associated with the element in the parent document, or something.
,
Dec 11
,
Dec 12
Do you have enough cycles to take this on Aleks?
,
Dec 12
I'll try. Neither embedding nor printing are my strengths, but I think I have some credits from the time I helped out the printing team. If I can just remember my contact... They are doing some funny stuff for OOPIF, such as shipping bitmaps for iframes between renderers...
,
Dec 12
Thanks Aleks!
,
Dec 12
@mstensho, what do you mean by re-attach. Which code does this? > This is very serious for LayoutNG, since we then always re-attach the entire > document when entering printing (LayoutNG doesn't support printing, so we need > to fall back to legacy layout, and rebuild everything).
,
Dec 12
By re-attach I mean when DOM nodes regenerate their LayoutObjects, i.e. Node::ReattachLayoutTree() and friends.
,
Dec 14
This attached example is also broken in Legacy. PASS does not get printed.
,
Dec 17
Yes, it fails in legacy too. This is bug isn't engine-specific, but hurts more in NG, because then we always reattach. I made a testcase that fails in both engines. Web test printing/iframe-svg-in-object-print.html only fails in NG.
,
Dec 17
Related: this used to fail in Legacy too https://bugs.chromium.org/p/chromium/issues/detail?id=838760
,
Dec 17
,
Dec 27
I spent a few days looking at it. I can't fix it without help, and it is unclear what the right approach is. Analysis and recommendation below. Reassigning to eae, can you decide which option looks most promising. The problem: OBJECT tags do not print in NG. There is no clean solution I can find. Analysis: http://localhost:9999/printing/iframe-svg-in-object-print.html The test case is more complex than necessary. circle.svg LayoutTree gets discarded when we switch to the print mode, and never gets regenerated. There are 3 Documents being printed: iframe-svg-in-object-print.html HTMLFrameOwnerElement -> subframe-svg-in-object.html subframe-svg-in-object.html HTMLPlugInElement -> circle.svg circle.svg When printing starts, all elements get Element::SetNeedsReattachLayoutTree() because they need to be switched from NG to Legacy layout tree. Node::DetachLayoutTree gets called on HTMLFrameOwnerElement and HTMLPlugInElement. On Detach, HTMLFrameOwnerElement and HTMLPlugInElement dispose the LocalFrameView. HTMLFrameOwnerElement regenerates LocalFrameView on AttachLayoutTree. HTMLPlugInElement posts a timer-fired task to reload the document. The task does not fire before printing completes, and HTMLPlugInElement remains empty. Fixes: There are 3 ways in which this bug could be fixed. 1) Implement printing in LayoutNG. According to eae, ~4 weeks worth of work, but not problematic. 2) Try to preserve HTMLPlugInElement's element tree. I coded a hacky attempt at doing this in https://chromium-review.googlesource.com/c/chromium/src/+/1389118 Current patch has 6 weird test failures (timeouts, crashes), and my confidence that fix will eventually work is low. I think the old code is fragile, and messing with the lifetime of EmbeddedContentView wrt HTMLPlugInElement causes unexplained behavior. 3) Complete reload task before printing. My knowledge of task code is limited, and I do not know how to do this. Maybe someone else knows? My fix recommendations are 1 or 3. Happy to help with either, but I can't do it without help.
,
Dec 27
Thanks for the detailed analysis and write up! I agree that option 2 seems quite fragile and option 3 sounds like a lot of throw-away work. Option 1 sounds like the best option to me but let's see what our friends on the printing team have to say.
,
Dec 27
Printing in LayoutNG without block fragmentation?
,
Dec 27
Oh right, of course that won't work without block fragmentation. I knew it sounded too simple!
,
Dec 27
Thank you for your analysis BTW, Aleks. To me it seems strange that we reload the child document just because we reattach the layout tree. I really think this operation should be idempotent. Here's a demo that proves that it's not so in Chrome: http://morten.stenshorne.net/bugs/crbug-913929-demo.html In Chrome, an OBJECT element with text/html contents gets reloaded when the layout object is re-attached. An IFRAME, on the other hand, doesn't reload the document. Having to load the whole thing (option 3) may indeed be necessary in some cases, but in this case, we should really preserve the child document during reattachment (option 2). Reloading seems like a waste of bandwidth and also a spec violation (but I haven't checked). Note that the demo doesn't use printing, but I believe it illustrates the actual bug that has to be fixed. So I think we should go for option 2.
,
Dec 28
Option 2 was my initial favorite too, that is why I tried it. My fix fixed our bug, but it opened a Pandora's box of gotchas. I am not sure whether getting this right is possible, or whether it violates some fundamental legacy assumptions. I'll work on it slowly, in parallel with other bugs. I have a feeling I can easily spend a month chasing all the loose ends on this, and working just on it will drive me batty. Is there anyone familiar with HTMLPlugInElement and EmbeddedContentView. Anyone around who did plugin work? Maybe site isolation had to deal with this? The work so far is at: https://chromium-review.googlesource.com/c/chromium/src/+/1389118 I started by modifying: HTMLPluginElement::DetachLayoutTree and AttachLayoutTree not to reload. Reload was done implicitly by setting EmbeddedContentView to null. Instead, I detach the layout tree of EmbeddedContentView on detach, and regenerated it on attach. This also required a change in LayoutEmbeddedContent::WillBeDestroyed. Detaching the layout tree would call this, and it would also trigger a reload inside its HTMLPlugInElement element by resetting its EmbeddedContentView. This means that PlugIn documents have never discarded layout tree without reloading the entire document. Every discard would disconnect the plugin from its owner. And this was done explicitly, reaching from inside Plugin's layout tree to make sure it is disconnected from its frame. The interaction between LayoutEmbeddedContent and HTMLPlugInElement is a bit of a mystery.
,
Today
(12 hours ago)
Reassigning to @mstensho, per slack chat. My work so far has been detailed in the comments above. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by e...@chromium.org
, Dec 11