Last Comment Bug 739004 - Switching print orientation in print preview makes canvas disappear because it clones the static document
: Switching print orientation in print preview makes canvas disappear because i...
Status: VERIFIED FIXED
[qa!]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla14
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 302566
  Show dependency treegraph
 
Reported: 2012-03-24 17:40 PDT by Boris Zbarsky [:bz]
Modified: 2012-04-26 06:57 PDT (History)
8 users (show)
bzbarsky: in‑testsuite?
moz.mihaelav: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
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). (1.69 KB, patch)
2012-03-24 17:46 PDT, Boris Zbarsky [:bz]
bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-03-24 17:40:25 PDT
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?
Comment 1 Boris Zbarsky [:bz] 2012-03-24 17:46:21 PDT
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).
Comment 2 Daniel Holbert [:dholbert] 2012-03-24 18:04:34 PDT
FWIW, the same thing happens when you change the scale, e.g. from "Shrink to fit" to "50%".
Comment 3 Boris Zbarsky [:bz] 2012-03-24 18:57:37 PDT
Yeah, that does the same thing under the hood.
Comment 4 Olli Pettay [:smaug] 2012-03-26 09:32:42 PDT
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?
Comment 5 Olli Pettay [:smaug] 2012-03-26 10:25:08 PDT
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.
Comment 6 Olli Pettay [:smaug] 2012-03-26 10:25:28 PDT
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.
Comment 7 Olli Pettay [:smaug] 2012-03-26 10:32:23 PDT
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.
Comment 8 Olli Pettay [:smaug] 2012-03-26 11:27:52 PDT
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)
Comment 9 Boris Zbarsky [:bz] 2012-03-26 16:11:33 PDT
Made those changes, and pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/00e08da6ff72
Comment 10 Boris Zbarsky [:bz] 2012-03-26 16:14:06 PDT
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
Comment 11 Ed Morley [:emorley] 2012-03-27 05:20:28 PDT
https://hg.mozilla.org/mozilla-central/rev/00e08da6ff72
Comment 12 Alex Keybl [:akeybl] 2012-03-27 13:33:34 PDT
We'll wait for this to bake a couple of days on central before triaging.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-29 15:02:10 PDT
Boris: what potential impact does this have on non-print preview code paths? I see a few other callers of CreateStaticClone...
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-29 15:19:10 PDT
Oh, there are other CreateStaticClones. The others all seem to be separate, so I guess printing is the only call to the nsIDocument one.
Comment 15 Boris Zbarsky [:bz] 2012-03-30 00:12:03 PDT
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.
Comment 16 Boris Zbarsky [:bz] 2012-03-30 00:13:15 PDT
And the other CreateStaticClones are all called via the document one.  This stuff is all used only for printing.
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-30 12:45:46 PDT
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.
Comment 18 Mihaela Velimiroviciu (:mihaelav) 2012-04-02 05:46:57 PDT
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
Comment 19 Alex Keybl [:akeybl] 2012-04-02 10:43:50 PDT
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).
Comment 21 Ioana (away) 2012-04-20 05:22:06 PDT
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.
Comment 22 Ioana (away) 2012-04-26 06:57:02 PDT
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

Note You need to log in before you can comment on or make changes to this bug.