New issue
Advanced search Search tips

Issue 913929 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Attaching OBJECT during printing doesn't work

Project Member Reported by mstensho@chromium.org, Dec 11

Issue description

Attaching 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.
 
tc.html
249 bytes View Download
Labels: -Pri-2 Pri-1
Components: Blink>Layout
Labels: LayoutNG
Owner: atotic@chromium.org
Do you have enough cycles to take this on Aleks?
Status: Assigned (was: Available)
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...
Thanks Aleks!
@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).
By re-attach I mean when DOM nodes regenerate their LayoutObjects, i.e. Node::ReattachLayoutTree() and friends.
This attached example is also broken in Legacy. PASS does not get printed.
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.
Related: this used to fail in Legacy too

https://bugs.chromium.org/p/chromium/issues/detail?id=838760
Status: Started (was: Assigned)
Owner: e...@chromium.org
Status: Assigned (was: Started)
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.

Cc: thestig@chromium.org
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.
Printing in LayoutNG without block fragmentation?
Oh right, of course that won't work without block fragmentation. I knew it sounded too simple!
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.
Owner: atotic@chromium.org
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.



Comment 18 by atotic@chromium.org, Today (12 hours ago)

Owner: mstensho@chromium.org
Reassigning to @mstensho, per slack chat. My work so far has been detailed in the comments above.

Sign in to add a comment