Switching print orientation in print preview makes canvas disappear because it clones the static document

VERIFIED FIXED in Firefox 12

Status

()

Core
DOM
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla14
Points:
---
Bug Flags:
in-testsuite ?
in-litmus +

Firefox Tracking Flags

(firefox12+ verified, firefox13+ verified)

Details

([qa!])

Attachments

(1 attachment)

Olli, when we switch print orientation in print preview, we clone the static document, and set the mOriginalDocument of the new clone to the original static document, not to its mOriginalDocument.  This causes the problem described in bug 734406 comment 12.

I could fix this in the frame constructor code by walking the mOriginalDocument "chain", but this looks like a bug in the cloning code to me.  In particular, repeatedly switching modes looks like it would use up more and more memory...

Thoughts?
Created attachment 609048 [details] [diff] [review]
When cloning a static clone, set the mOriginalDocument of the new clone to the mOriginalDocument of the thing being cloned, not to the thing being cloned (which is not an original document at all).
Attachment #609048 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Whiteboard: [need review]
FWIW, the same thing happens when you change the scale, e.g. from "Shrink to fit" to "50%".
Yeah, that does the same thing under the hood.
Comment on attachment 609048 [details] [diff] [review]
When cloning a static clone, set the mOriginalDocument of the new clone to the mOriginalDocument of the thing being cloned, not to the thing being cloned (which is not an original document at all).


>   nsCOMPtr<nsIDocument> clonedDoc;
>   if (NS_SUCCEEDED(rv)) {
>     clonedDoc = do_QueryInterface(clonedNode);
>+    // XXXbz why do we need this clonedDOMDoc variable here?
Looks like a left over from something. Please remove it.


>     nsCOMPtr<nsIDOMDocument> clonedDOMDoc = do_QueryInterface(clonedDoc);
>     if (clonedDOMDoc) {
>-      clonedDoc->mOriginalDocument = this;
>+      if (IsStaticDocument()) {
>+        clonedDoc->mOriginalDocument = mOriginalDocument;
>+      } else {
>+        clonedDoc->mOriginalDocument = this;
>+      }

IIRC I did this on purpose, so that animations get printed the same way even if you change setting which need to reclone.

Could we add a new method to nsIDocument which basically traverses the document chain?
Attachment #609048 - Flags: review?(bugs) → review-
Comment on attachment 609048 [details] [diff] [review]
When cloning a static clone, set the mOriginalDocument of the new clone to the mOriginalDocument of the thing being cloned, not to the thing being cloned (which is not an original document at all).


>     clonedDoc = do_QueryInterface(clonedNode);
>+    // XXXbz why do we need this clonedDOMDoc variable here?
>     nsCOMPtr<nsIDOMDocument> clonedDOMDoc = do_QueryInterface(clonedDoc);
>     if (clonedDOMDoc) {
Remove this.



>-      clonedDoc->mOriginalDocument = this;
>+      if (IsStaticDocument()) {
>+        clonedDoc->mOriginalDocument = mOriginalDocument;
>+      } else {
>+        clonedDoc->mOriginalDocument = this;
>+      }
Ok, silly me. Cloning is still doing the right thing.
Attachment #609048 - Flags: review- → review+
Comment on attachment 609048 [details] [diff] [review]
When cloning a static clone, set the mOriginalDocument of the new clone to the mOriginalDocument of the thing being cloned, not to the thing being cloned (which is not an original document at all).

Er, let me verify something.
Attachment #609048 - Flags: review+
Attachment #609048 - Flags: review+
Comment on attachment 609048 [details] [diff] [review]
When cloning a static clone, set the mOriginalDocument of the new clone to the mOriginalDocument of the thing being cloned, not to the thing being cloned (which is not an original document at all).

No, still  one thing to verify.
Attachment #609048 - Flags: review+
Comment on attachment 609048 [details] [diff] [review]
When cloning a static clone, set the mOriginalDocument of the new clone to the mOriginalDocument of the thing being cloned, not to the thing being cloned (which is not an original document at all).

>   nsCOMPtr<nsIDocument> clonedDoc;
>   if (NS_SUCCEEDED(rv)) {
>     clonedDoc = do_QueryInterface(clonedNode);
>+    // XXXbz why do we need this clonedDOMDoc variable here?
>     nsCOMPtr<nsIDOMDocument> clonedDOMDoc = do_QueryInterface(clonedDoc);
Just remove clonedDOMDoc if it is not needed



>     if (clonedDOMDoc) {
>-      clonedDoc->mOriginalDocument = this;
>+      if (IsStaticDocument()) {
>+        clonedDoc->mOriginalDocument = mOriginalDocument;
>+      } else {
>+        clonedDoc->mOriginalDocument = this;
>+      }


Could you perhaps remove the loop in PresShell::SetPrefNoScriptRule()
which tries to find the real original document.

The patch should actually fix few other bugs too :)
(Which aren't just visible atm because our print preview UI doesn't expose certain functionality)
Attachment #609048 - Flags: review+
Made those changes, and pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/00e08da6ff72
Flags: in-testsuite?
Flags: in-litmus?
Whiteboard: [need review]
Target Milestone: --- → mozilla14
Comment on attachment 609048 [details] [diff] [review]
When cloning a static clone, set the mOriginalDocument of the new clone to the mOriginalDocument of the thing being cloned, not to the thing being cloned (which is not an original document at all).

[Approval Request Comment]
Regression caused by (bug #): 302566
User impact if declined: Changing print settings in print preview will make
   canvas elements disappear.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):  I think this is fairly
   low-risk.  This was clearly a bug in the way we set mOriginalDocument....
   An alternative would be backing out bug 302566 or me writing a branch-only
   patch for this bug to fix this via looping over original documents in the
   frame constructor.
String changes made by this patch:  None
Attachment #609048 - Flags: approval-mozilla-beta?
Attachment #609048 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/00e08da6ff72
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
We'll wait for this to bake a couple of days on central before triaging.
Boris: what potential impact does this have on non-print preview code paths? I see a few other callers of CreateStaticClone...
tracking-firefox12: --- → +
Oh, there are other CreateStaticClones. The others all seem to be separate, so I guess printing is the only call to the nsIDocument one.
This is only an issue when cloning already-static documents, so the only way to get there is via print preview.  It _might_ affect printing from print preview, though.
And the other CreateStaticClones are all called via the document one.  This stuff is all used only for printing.
OK, we wanted to clarify the scope of impact in the triage meeting. Given that it's limited to print preview/printing, I think we should approve this for aurora and beta, as discussed.
Added the 'Switch print orientation for pages containing canvas elements' test in Litmus under Firefox>Aurora>Aurora Full Functional Tests (FFTs)>FX Aurora FFT>Printing
Flags: in-litmus? → in-litmus+

Updated

5 years ago
tracking-firefox13: --- → +
Comment on attachment 609048 [details] [diff] [review]
When cloning a static clone, set the mOriginalDocument of the new clone to the mOriginalDocument of the thing being cloned, not to the thing being cloned (which is not an original document at all).

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17)
> OK, we wanted to clarify the scope of impact in the triage meeting. Given
> that it's limited to print preview/printing, I think we should approve this
> for aurora and beta, as discussed.

Agreed - let's move forward with landing this asap on Beta 12. Our next beta build is tomorrow (Tuesday 4/3).
Attachment #609048 - Flags: approval-mozilla-beta?
Attachment #609048 - Flags: approval-mozilla-beta+
Attachment #609048 - Flags: approval-mozilla-aurora?
Attachment #609048 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/670caaf9d22b
http://hg.mozilla.org/releases/mozilla-beta/rev/2a158d671b2f

Thanks for the approvals!
status-firefox12: --- → fixed
status-firefox13: --- → fixed
Whiteboard: [qa+]

Comment 21

5 years ago
Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
BuildID: 20120417165043

Verified as fixed with the steps from bug 734406 comment 12.
status-firefox12: fixed → verified

Comment 22

5 years ago
Verified as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:13.0) Gecko/20100101 Firefox/13.0
BuildID: 20120425123149
Status: RESOLVED → VERIFIED
status-firefox13: fixed → verified
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.