337356 - Page zoom breaks if content policy accesses frame's document

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Wladimir Palant]

This issue has been reported for Adblock Plus 0.7 - if you zoom a page with a frame and then reload it the frame will show text in normal size while the containing page is still zoomed. Attaching minimized test case.

Comment 1

[Wladimir Palant]

Attachment: Minimized content policy

This content policy does only one thing - it accesses node.contentWindow.document for frame nodes, that's enough to break page zoom. Simply save it as TestPolicy.js and drop it into the application's components directory. Then remove compreg.dat in your profile to make the component register.

Comment 2

[Wladimir Palant]

Attachment: Minimized testcase

This testcase contains a frame with some text. To reproduce the bug:

1. Install content policy component from attachment above
2. Go to this page (a JavaScript exception will occur because the content policy tries to access a document that isn't there yet)
3. Press Ctrl-+ a few times to zoom the page
4. Press "Reload"

You will see that the text outside the frame is zoomed while text inside it isn't.

I was able to reproduce it in Firefox 1.5.0.3, in a current trunk nightly and in Firefox 1.5.0.4 build 2006050817 where bug 336875 should be fixed (so this isn't the same issue).

Comment 3

[Wladimir Palant]

Forgot to mention: the testcase won't work with Adblock Plus 0.7 because it uses a data: URL that will be ignored.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yep.  Zoom is gotten from the previous content viewer, or from the parent if there isn't one.  You're forcing content viewer creation when you try to get the document from a window that doesn't have one yet, and CreateAboutBlankContentViewer doesn't copy zoom state, along with a bunch of other things.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, I was wrong.  CreateAboutBlankContentViewer does copy state.  But SetTextZoom and GetTextZoom on DocumentViewerImpl are broken -- they only work if there is an mPresContext, which is not going to be the case for the about:blank viewer here.

So the viewer should probably store the text zoom in a member, and possibly set it on the prescontext when creating it?

Comment 6

[Martijn Wargers (dead)]

Attachment: patch

So something like this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

That looks about right, yes.  Does it work?

Comment 8

[Martijn Wargers (dead)]

Yes, it seems to work. I'll recheck with backing out the patch.

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 222355 [details] [diff] [review]
patch

Yes, backing out the patch makes the bug appear again.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 222355 [details] [diff] [review]
patch

Add an assert in GetTextZoom that !mPresContext || mPresContext->TextZoom() == mTextZoom, and r+sr=bzbarsky

Comment 11

[Martijn Wargers (dead)]

Attachment: patch2, with assertion

Boris, something like this?

Comment 12

[Martijn Wargers (dead)]

Attachment: patch2, with assertion

Er, like this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 223839 [details] [diff] [review]
patch2, with assertion

You want:

+  NS_ASSERTION(!mPresContext || mPresContext->TextZoom() == mTextZoom, 
+               "mPresContext->TextZoom() != mTextZoom");

Comment 14

[Martijn Wargers (dead)]

Yeah, sorry about continuously wrong patches here. I'll do what you say in comment 13 and will check that in (hopefully) tomorrow.

Comment 15

[Martijn Wargers (dead)]

Attachment: patch, with the right assert this time

Comment 16

[Martijn Wargers (dead)]

Checked that patch in. Sorry for the delay.

Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.490; previous revision: 1.489
done