202029 - Second PresShell holds invalid reference to View Manager

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[John Keiser (jkeiser)]

PresShells in a second presentation hold an invalid pointer to ViewManager which
is covered up in printing by holding a strong pointer to it elsewhere.  The problem:

(1) there is one document viewer for the document/docshell
(2) PresShell is one per *presentation*
(3) ViewManager is one per *presentation*

PresShell holds a weak reference to its ViewManager, counting on DocumentViewer
holding the strong ref.  But DocumentViewer only holds the ViewManager for the
first presentation, leaving any new, trusting PresShells empty-handed.

An additional problem is the order of destruction ... if the view manager goes
away before the frames are destroyed (which happens if PresShell holds the only
and final reference to the view manager), we understandably crash.

Also, while I'm at it I'd like to move the CreateStyleSet() method from
nsIDocumentViewerPrint to nsIDocumentViewer, since it's not really
print-specific.  The reason for combining these changes is that I needed them
both for my app :)

Comment 1

[John Keiser (jkeiser)]

OK, looking at the code more deeply, I *think* I'm wrong in my assumption that
you only have one DocumentViewer.  Printing simply hacked around it so that it
worked *without* a DocumentViewer.  If this is the case, printing just needs to
be fixed, not the reference-holding.

Leaving open in case I'm wrong yet again.  The patch I have in my tree works ok,
I'll attach that for posterity.

Comment 2

[John Keiser (jkeiser)]

Attachment: Fixes PresShell reference problem

I haven't checked for leaks here yet, they may exist but since
PresShell::Destroy() is linked to DocumentViewer::Destroy() and callers have to
call that rather than wait for DocumentViewer to actually be deleted, a leak is
unlikely.

Comment 3

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

I had assumed that we were eventually going to do away with multiple
presentations for the same document. Why do you need that?

Comment 4

[John Keiser (jkeiser)]

Well, that's a big assumption to make given printing, but I need a second
presentation in order to do similar stuff to printing.  Any time you want to lay
out a copy of a document differently you need this.  But the point is, as long
as we have multiple presentations and use them, which we currently do, we should
do it right.

As to the question of multiple presentaitons, I have discovered we're a long way
from needing to have only one presentation--a second one is not hard to create
and dump out.  Until we decide that we want to combine frames and content nodes
together into the same tree (same objects), the cost of keeping multiple
presentations around is low enough that we may as well do it.  At that point
we'll also have to decide whether it's ok to clone the document for printing (I
think it's ok personally, it's just a decision to be taken, and not to be taken
lightly, as we'd have to fix cloning to work right).  But there's no point in it
unless there is some real savings we plan to take out of it.  And I haven't
heard of any bold plans that would require only one presentation yet.

Comment 5

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

> Until we decide that we want to combine frames and content nodes
> together into the same tree (same objects)

We won't ever do that, because of 'display' and other issues.

> the cost of keeping multiple presentations around is low enough that we may as
> well do it

I basically agree but others don't.

The main problem I see is that we currently don't have a compelling application
for multiple presentations. That means that multi-presentation handling is very
poorly maintained. If we're serious about multiple presentations then there
should be some way to use it in Seamonkey/Phoenix that gives us something to
test and some incentive to keep it working. Printing is not it. Ideally we'd
have something that exploited dynamism. Window splitting in the HTML composer
would suffice.

Comment 6

[John Keiser (jkeiser)]

Yeah, I think truly compelling applications would all depend on multiple
*interactive* presentations, which I suspect we do *not* support (one of these
days I'm going to have to test that theory--perhaps we can actually make that
leap and there are just a few issues to be worked out like fixing all content
nodes that do GetPresShellAt(0) to notify frames of changes).  Our multiple
presentation support currently basically involves a primary presentation, and
having any other presentations be secondary, non-interactive, like printing.

We *could* have a split view in Composer where the viewable view could not be
clicked on or worked with, but that would be rather useless given the
WYSIWYG-ness of the actual editor part of the window.

The only current argument *for* them is multiple presentations make operations
like printing faster and more reliable, and that it is a low enough cost of
upkeep that we may as well do it.  If someone came along and said "look, if we
only had one presentation then we could greatly simplify our architecture in
such-and-such a way," it would probably be worth jumping on that bandwagon.

Comment 7

[John Keiser (jkeiser)]

OK, in attempting to construct a DocumentViewer for my second presentation I
have learned just how far out into no-man's-land DV actually is.  It looks like
it was designed to create and manage a presentation, but it also has hooks (like
SyncSubDocumentMap) that basically perform DocShell functions.

Ultimately, DocumentViewer needs a big spanking.  In the meantime, since
printing is already using second presentations with PresShell (and hacking
around this problem) and others who do the same will be doing this also, I'd
like to fix this mess.

I just noticed my class includes nsIPresShell::GetCanvasFrame(), which is a
refactoring I did because I needed that particular frame and several other
places use it as well; I'll include those changes as well before getting review.
 I have tested this patch though and verified that it does *not* leak
ViewManagers, so we're fine on that score.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> I had assumed that we were eventually going to do away with multiple
> presentations for the same document.

Didn't we do that already?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, we did.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dholbert, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[Daniel Holbert [:dholbert]]

I think this is no longer an issue, per comment 8 - 9.