Closed Bug 337356 Opened 14 years ago Closed 14 years ago

Page zoom breaks if content policy accesses frame's document


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

Windows XP
Not set





(Reporter: gaubugzilla, Assigned: martijn.martijn)




(3 files, 3 obsolete files)

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.
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.
Attached file 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, in a current trunk nightly and in Firefox build 2006050817 where bug 336875 should be fixed (so this isn't the same issue).
Forgot to mention: the testcase won't work with Adblock Plus 0.7 because it uses a data: URL that will be ignored.
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.
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?
Keywords: helpwanted
Attached patch patch (obsolete) — Splinter Review
So something like this?
That looks about right, yes.  Does it work?
Yes, it seems to work. I'll recheck with backing out the patch.
Comment on attachment 222355 [details] [diff] [review]

Yes, backing out the patch makes the bug appear again.
Attachment #222355 - Flags: review?(bzbarsky)
Comment on attachment 222355 [details] [diff] [review]

Add an assert in GetTextZoom that !mPresContext || mPresContext->TextZoom() == mTextZoom, and r+sr=bzbarsky
Attachment #222355 - Flags: superreview+
Attachment #222355 - Flags: review?(bzbarsky)
Attachment #222355 - Flags: review+
Assignee: general → martijn.martijn
Flags: blocking1.9a2+
Attached patch patch2, with assertion (obsolete) — Splinter Review
Boris, something like this?
Attachment #222355 - Attachment is obsolete: true
Attached patch patch2, with assertion (obsolete) — Splinter Review
Er, like this.
Attachment #223807 - Attachment is obsolete: true
Comment on attachment 223839 [details] [diff] [review]
patch2, with assertion

You want:

+  NS_ASSERTION(!mPresContext || mPresContext->TextZoom() == mTextZoom, 
+               "mPresContext->TextZoom() != mTextZoom");
Attachment #223839 - Flags: review-
Yeah, sorry about continuously wrong patches here. I'll do what you say in comment 13 and will check that in (hopefully) tomorrow.
Attachment #223839 - Attachment is obsolete: true
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
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: helpwanted
Blocks: abp
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.