201767 - Printing pages with form elements causes bad behavior

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Mike Kaply [:mkaply]]

After the drop of the new scrollbar code, web pages with listboxes don't print
correctly.

After printing, new scrollbars are present where there were none, and listboxes
get resized on screen (this is on Windows)

On OS/2, we are seeing crashes when we print pages with form elements.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: non-forms testcase

This testcase shows that these problems have actually been around for a while,
and I just made them worse by switching to nsGfxScrollFrame.

This testcase is just a use of 'overflow:scroll'. Printing this seems to
produce various ill effects. On 1.3final it produces the weird extra scrollbars
mkaply mentioned. On 1.4trunk with Classic theme it crashes in
nsNativeThemeGTK:

  aContext->GetDrawingSurface((nsDrawingSurface*)&surface);
  GdkWindow* window = (GdkWindow*) surface->GetDrawable();

'surface' is null, presumably because printing doesn't use offscreen drawing
surfaces.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With Modern theme, this testcase doesn't crash but it does get the extra
scrollbars. It's kind of cool, every time you print a couple more scrollbars get
added. I think this must be anonymous content being added by frame construction
for the print presentation and not being removed.

jkeiser: this could be an argument for one presentation per document. Handling
per-presentation anonymous content seems hard.

Anyway, how to fix? There seem to be two problems:
-- what to do about nsNativeThemeGTK during printing. Just disable the whole
shebang and go with the default style rules? In theory we could print by getting
GTK to render into a temporary pixmap and then blitting that to the print
device. Note that if we turned on GTK themes for general HTML widgets this
problem would bite us even more.
-- what to do about the anonymous content. I think perhaps Gfx frame
construction should not create the anonymous scrollbars at all if we're in a
print presentation. This would actually resolve the nsNativeThemeGTK issue for
the time being, too.

Comments?

Comment 3

[John Keiser (jkeiser)]

Wow, that *is* fun.

Frame-based anonymous content lives in the pres shell.  If this is XBL anonymous
content, I can see how maybe a problem will occur (though I don't know enough
about it to be detailed).  The XBL binding manager, which stores the anonymous
content, resides in the document rather than the presentation, and so is shared
like you say.  Either XBL needs to be fixed to work with multiple presentations
(which, given the way it works already, might not be terribly hard), or we need
to abandon multiple presentations and rewrite the printing loader to clone the
document, create a pres shell and print that, which is sure to cause unforeseen
problems due to PageContentFrame.

Are the scrollbars actually anonymous content?

Comment 4

[Mike Kaply [:mkaply]]

We're also seeing crashes when editing pages with scrollbars. Could this be 
related as well (select edit page when viewing a form page)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Scrollbars are frame anonymous content. I'm not sure exactly what is going on
here, I'll try to find out.

Comment 6

[David Hyatt]

My humble suggestion for how to do printing:

(1) Ditch multiple presentations and have just one presshell per document.
(2) When the time comes to print, make it synchronous and do the following
  (a) Determine the new media type list and re-resolve style on your frame tree.
  (b) Spool to the printer synchronously.
  (c) Re-resolve style back to screen.

BTW, XBL will never work with multiple presentations, unless script objects also are per-
presentation (since event handlers and properties are installed on DOM objects and not held at the 
presentation level).  Trying to have multiple presentations is IMO simply a needless 
complication of the layout engine and one of the worst mistakes made in Gecko.

Comment 7

[John Keiser (jkeiser)]

Note that printing currently needs a different device context, rendering
context, and has special stuff in the pres context, so we have a ways to go
before we can render using the same frame tree.  I don't think it's worth it to
hack something like that up anyway; if we have to go back to one presentation
per document, we should just clone the document, dump it into a shell and dump
out to a completely new presentation.  Then printing (or another secondary
presentation) doesn't have to block your UI either.  There's a long way to go
before we get *there* either.  Printing initialization needs a lot of cleanup
one way or another.

This sounds like a compelling enough argument to drop multiple presentations--I
have been looking for a good excuse anyway.  I can't get to a printing
initialization cleanup until after 1.5, however (I am redoing the layout half of
printing instead, which also needed cleanup).

At any rate, I bet this bug can be solved another way--if the problem is really
rebinding the xbl anonymous children, we can either prevent the rebinding in an
alternate pres shell (a hack, but it's probably safe for now) or make sure that
when the binding goes away, the scrollbars do too.

Comment 8

[John Keiser (jkeiser)]

oh, I missed roc's comment about them being frame anonymous content.  OK, we'll
take the xbl discussion elsewhere ;)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, so if we are going to commit to a single presentation per document, then we
technically don't need to pass nsIPresContext* in every method call (as hyatt
ranted about in his blog). A frame could obtain it via
mContent->GetDocument()->GetPresContext(). This could be done as part of the
nsIFrame cleanup patch. Whaddayathink?

Comment 10

[John Keiser (jkeiser)]

Yeah, that's pretty reasonable, especially given how seldom it's actually
*used*.  But let's not drop it just yet--we *do* have multiple presentations
right now, and until we actually fix printing let's not drop support for them ;)

We don't need the pres context in every method call now anyway.  I bet if you
stored mPresContext in all frames except textframes and removed nsIPresContext*
from every method call you would get a bloat win.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't actually use more than one presentation at the same time, right? Can't
we have the following transition strategy:
1) add an mPresContext field and an inline GetPresContext() getter to nsIDocument
2) add a SetPresContext() method to nsIDocument
3) call SetPresContext() when the presshell is set up
4) have printing call SetPresContext() to the print presentation, and then set
it back when printing is done

We're going to check in a new simplified nsIFrame API fairly soon, and it would
be good to get rid of the nsIPresContext*s then rather than later.

Comment 12

[John Keiser (jkeiser)]

Sounds like it could work ... I think printing is synchronous at the moment, at
least the part that counts.

Comment 13

[David Hyatt]

Note that scrollbars are not done using XBL (their interior contents are done using XBL, but the 
scrollframe does the creation of the scrollbars themselves).  It's using the C++ scheme for 
anonymous content, which is totally different.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What seems to be happening is this:
-- switch to print presentation
-- create some anonymous scrollbar elements and their frames for the print
nsGfxScrollFrame
-- print nsGfxScrollFrame twiddles some attributes on its scrollbar
-- screen nsCSSFrameConstructor observes attribute change, decides to recreate
frames for the scrollbar
-- screen nsCSSFrameConstructor proceeds to add scrollbar frames for the print
nsGfxScrollFrame's scrollbars
-- of course, the screen nsGfxScrollFrame's scrollbars and scrollbar frames are
still there too

Easiest thing to do is for nsGfxScrollFrame to not create anonymous scrollbars
if it's in a print context (unless it's the viewport for print preview). Sound
reasonable? It leaves open the probability that other uses of anonymous content
could similarly get in trouble. I think the right long-term fix is to stick to a
single presentation; in this case, we'd be tearing down the screen frame model
and it wouldn't be active and notified when printing added scrollbars.

Comment 15

[John Keiser (jkeiser)]

Seems reasonable, we shouldn't need scrollbars in print anyway, you can't scroll
:)  Other approaches all involve detecting which pres shell the content is in,
which is not a trivial operation.

A single presentation is the way to go though, you're right.  Disabling
attribute notifications for anonymous content would suck.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So.. 

1) I know that _some_ part of printing is async.  How much?  Does printing
layout happen async?  Or just the painting?  How much would need to happen sync
if we did what comment 6 suggests?

2) If we plan to heavily use ReResolveStyleContext (like every time we print),
we'd better fix a bunch of issues that remain with it...  We may have to do this
anyway if we switch to non-blocking sheet loading, mind you.

3) Why is the _screen_ nsCSSFrameConstructor doing anything to the _print_
frames?  How'd that happen?

4) Should printing a page clobber all java plugins running on it?  If we
reframed to print, it would, unless we move plugins to content first (which we
should still do, mind you).

The short-term solution sounds fine, as a general "make printing even more
weird" kinda thing.. ;)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> 4) Should printing a page clobber all java plugins running on it?  If we
> reframed to print, it would, unless we move plugins to content first (which we
> should still do, mind you).

I'm not sure exactly how plugin printing is supposed to work, but definitely we
should move plugins to content so that their windows can survive reframes.

> Why is the _screen_ nsCSSFrameConstructor doing anything to the _print_
> frames?  How'd that happen?

The scrollbars for the print frames are anonymous content that's in the
document. All nsCSSFrameConstructors for the document observe all attribute
changes in the document, including for those scrollbars.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've been working on this for a while. Fixing the scrollbars was pretty easy.
But I've found another crasher print-previewing
http://www.mozilla.org/quality/browser/front-end/testcases/printing/widgets.html;
the view hierarchy is getting out of sync with the frame hierarchy because view
reparenting isn't happening at some point where it should. The problem seems to
be a combination of multi-page layout (using pages as continuing frames) and the
fact that that testcase doesn't close some of its FORM elements, which creates
an icky content/frame hierarchy.  It's really ugly.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix!

OK, here is the fix for both issues. The patch does a few things. It prevents
scrollbar anonymous content from being created in a print context unless the
scrollbar is for a viewport (as for the print preview window). It robustifies a
few places where nsGfxScrollFrame couldn't handle the absence of scrollbars. (I
do wonder why those places weren't triggered by the <input type="text">
scrollbar disabling.)

The patch also fixes another basically unrelated problem: I converted
nsHTMLContainerFrame view reparenting logic to walk frame child lists using
GetAdditionalChildListName. Unfortunately nsBlockFrame does not report its
overflowList via GetAdditionalChildListName! So frames that happened to be on
the overflowList at reparenting time were ignored and this could cause a bad
view hierarchy to be created under fairly random and rare conditions. I changed
nsBlockFrame to report its overflowList via GetAdditionalChildListName. A quick
glance at other users of that function makes me think they'll be OK with this
change and probably even desire it.

Now that I've finally tracked down this mess I'll attend to my review queue for
a while :-).

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I should also mention that while working on this, I discovered that it's
possible for scrollbars to be removed and added during reflow, which could
trigger pointless and expensive frame recreation for lazy scrollbars. So I made
it so when a scrollbar first becomes visible, we remove the 'lazy' attribute so
it will not be deconstructed if it becomes hidden again.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121159 [details] [diff] [review]
fix!

>   if (!aVisible) {
>      content->SetAttr(kNameSpaceID_None, nsXULAtoms::collapsed, NS_LITERAL_STRING("true"), PR_TRUE);
>   } else {
>+    // disable laziness; we never want to recreate these scrollbars again
>+    // once we've created them
>+    // disable laziness FIRST so only one recreation happens.
>+     content->UnsetAttr(kNameSpaceID_None, nsXULAtoms::lazy, PR_TRUE);
>      content->UnsetAttr(kNameSpaceID_None, nsXULAtoms::collapsed, PR_TRUE);

What's with the wacky indentation?  Fix that, please?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Hmmm... Tp did not look happy with this checkin... :(  The averages rose in
about this timeframe on both btek and monkey....

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could try backing out the unsetting of 'lazy'. Nothing else should be on the
Tp path. Unless iterating through a block's overflowList is expensive somewhere
where we don't need it.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Hmmmm.. are things in the overflow list present in some other frame list
somewhere too?

One place where we iterate over frame lists is style resolution; I'm just trying
to tell whether we're now iterating over extra stuff, or whether we used to not
iterate over things that we _should_ have iterated over.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> are things in the overflow list present in some other frame list somewhere too?

Surely not. mNextSibling would be clobbered

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe we should try backing out the UnsetAttr('lazy') before we spend a lot of
time examining code.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Sure, let's do that.  Maybe we can do it sometime after 1.4b is tagged?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yah

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Regarding comment 6 -- why not just clone the document?  Swapping media types
during reresolution and turning a "galley" frame tree into a paginated one and
back seems like unnecessary complication that's likely to lead to bugs.  The
DOM's Clone() method ought to copy the necessary state (e.g., form state), right?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Nope.  Clone() does not copy form state.  It copies default values, but not
current values.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could clone the tree at the content object level.

Comment 33

[John Keiser (jkeiser)]

However, cloning the document *should* copy current state IMO.  The spec on
cloneNode() says "user data is not carried over," so if that's what it's talking
about, we can always add a parameter to our internal clone method so that that
*will* happen.  Cloning the document is the right way to do it, as noted in
comment 7.

Comment 34

[WD]

I think this patch needs to be backed out.
With this patch, recent builds exhibit bug 204915 , which is a crasher.

If I back out this patch, there is no crash.