Closed Bug 288873 Opened 19 years ago Closed 19 years ago

content of iframe disappears after mouseover at print preview

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: linxspider, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(3 files)

when moving the mouse over an "iframe" in print preview, the content of it
disappears after returning to the page.

this seems to be a regression from Bug 284664
Attached file testcase
an example of the problem
In print preview, if I move the mouse outside the iframe I get a continuous
stream of:

###!!! ASSERTION: Mouse move must have some target content: 'targetElement',
file
/home/bzbarsky/mozilla/xlib/mozilla/content/events/src/nsEventStateManager.cpp,
line 2682

And when I open up print preview once the iframe has gone blank, I get:

###!!! ASSERTION: null window: 'mWindow', file
/home/bzbarsky/mozilla/xlib/mozilla/layout/base/nsDocumentViewer.cpp, line 1671
The assertions about no target for the mouse move are fairly straightforward ...
there is no content corresponding to the page frames and the viewport area
outside the pages, so no target content for mouse moves over those frames. This
should be fairly innocuous. I can patch that up so that GenerateMouseEnterExit
treats the mouse as over the root element if it can't find any other target content.

But how this manages to nuke the IFRAME at end of print preview, I'm not sure. I
do get this assertion:
###!!! ASSERTION: no document: 'doc', file
../../../../../content/xul/content/src/nsXULElement.cpp, line 2376
Break: at file ../../../../../content/xul/content/src/nsXULElement.cpp, line 2376
On the stack, some script is trying to execute a "cmd_undo".
This is a better testcase:

https://bugzilla.mozilla.org/attachment.cgi?id=179686

I've noticed that the IFRAME contents only get thrown away if we move the mouse
out of it during print preview.
Attached patch fixSplinter Review
The problem is that the ESM code to propagate mouse events down into
subdocuments was calling nsSubdocumentFrame::GetDocShell. This has the side
effect of loading the subdocument if it hasn't already started loading. In
particular it hooks up nsSubdocumentFrame::mFrameLoader to the frame loader.
Then nsSubdocumentFrame::Destroy thinks it's responsible for telling
mFrameLoader's document viewer to Hide() ... even though this document viewer
is the docviewer for the main presentation, and we're in the print preview
presentation. That would be OK because Hide() checks to see if it's in Print or
PrintPreview and ignores the hide request if so. Unfortunately the comments lie
and the check always returns PR_FALSE in subdocuments, because they don't have
a pointer to the print engine. So we go ahead and tear down the subdocument's
main presentation.

The approach I'm taking here is to simply not call GetDocShell with that chain
of nasty side effects ... instead we can get the subdocument's prescontext/ESM
by walking the view tree. This doesn't work in print/print preview because they
don't have the view trees hooked up, but who cares.

This patch also includes a fix to stop those assertions firing. And it
documents the lies in DocumentViewerImpl.
Attachment #179821 - Flags: superreview?(bzbarsky)
Attachment #179821 - Flags: review?(bzbarsky)
Comment on attachment 179821 [details] [diff] [review]
fix

I guess this is ok as wallpaper, but could you file a bug on fixing the
nsDocumentViewer code?	It should probably call internal methods that check its
parents too, not the frozen api functions it's calling now.

If you don't have time to fix that, let me know and I'll look for someone to do
it, or do it myself.

Once that's done, do we want to go back to using GetDocShell, or leave things
as they are?
Attachment #179821 - Flags: superreview?(bzbarsky)
Attachment #179821 - Flags: superreview+
Attachment #179821 - Flags: review?(bzbarsky)
Attachment #179821 - Flags: review+
Comment on attachment 179821 [details] [diff] [review]
fix

fairly straightforward fix to a major regression
Attachment #179821 - Flags: approval1.8b2?
There are really a couple of bugs here:
1) DocumentViewerImpl doesn't ignore Hide() in print/print preview for subdocuments.
2) nsSubDocumentFrame::GetDocShell shouldn't have the side effect of triggering
document load. It should be called ObtainDocShell or something like that, and if
necessary, a non-side-effecting GetDocShell should be added. In particular
nsSubDocumentFrame shouldn't be deciding whether to call Hide() based on whether
GetDocShell was ever called or not.

Honestly, I don't fully understand the lifetime issues and relationships between
the docshell, the documentviewer, the frameloader, and how the print engine
massages them.

But you're right that this fix isn't great. Adding new API as a band-aid is not
a good idea. I'll submit another patch which is less band-aid-y and at least
doesn't add new API.
Attached patch fix #2Splinter Review
This approach just makes the subdocument frame not call Hide() if it didn't
create the presentation in ShowDocShell. That seems reasonable. It certainly
fixes the bug and doesn't require any new interface.
Attachment #179891 - Flags: superreview?(bzbarsky)
Attachment #179891 - Flags: review?(bzbarsky)
> Honestly, I don't fully understand the lifetime issues and relationships
> between the docshell, the documentviewer, the frameloader, and how the print
> engine massages them.

That's because the relationships are crappy.  The document viewer is sort of a
per-presentation object, except there's one particular one that the docshell
holds a reference to... The rest of the mess is corollaries of this.  :(
Comment on attachment 179891 [details] [diff] [review]
fix #2

Yeah, this is better.  Maybe s/ObtainDocShell/EnsureDocShell/ or something?
Attachment #179891 - Flags: superreview?(bzbarsky)
Attachment #179891 - Flags: superreview+
Attachment #179891 - Flags: review?(bzbarsky)
Attachment #179891 - Flags: review+
Comment on attachment 179891 [details] [diff] [review]
fix #2

okay, this is the preferred fix for this unfortunate regression.
Attachment #179891 - Flags: approval1.8b2?
Comment on attachment 179891 [details] [diff] [review]
fix #2

a=asa for 1.8b2 checkin.
Attachment #179891 - Flags: approval1.8b2? → approval1.8b2+
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
is this bug somehow related to Bug 176611 ?
Note: this caused bug 289719
Depends on: 289719
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: