1334346 - DocGroup is wrong for print preview documents

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

I'm trying to turn on assertions to ensure that a runnable labeled with TabGroup G will never touch any other TabGroups besides G. I'm getting assertions in print preview tests. Here's what seems to be happening.

1. First, we clone the document to be previewed.
2. During cloning, we call SetScopeObject on the cloned document with the scope object of the original document [1]. The cloned document sets its mDocGroup to the docgroup of the original document [2].
3. The print preview code then calls SetDOMDocument on the cloned document [3]. SetDOMDocument eventually calls SetScopeObject again, now with the scope from the print preview docshell. However, we already have a cached mDocGroup, which we don't change [4].

The result is that the cloned document ends up in a DocGroup that doesn't belong to its window's TabGroup. I hacked around this in the most obvious possible way since I don't really understand this code. Suggestions for a better way are welcome.

[1] http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/dom/base/nsDocument.cpp#9222
[2] http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/dom/base/nsDocument.cpp#4515
[3] http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/layout/printing/nsPrintObject.cpp#94
[4] http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/dom/base/nsDocument.cpp#4512

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Since static cloning happens right before http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/layout/printing/nsPrintObject.cpp#94
I think we should just not set scriptHandlingObject initially when doing a static clone.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Perhaps worth to assert in document's GetParentObject or GetScopeObject that we don't try to access it during cloning.
If we do, we have other things to fix too.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Olli Pettay [:smaug] from comment #1)
> Since static cloning happens right before
> http://searchfox.org/mozilla-central/rev/
> 7da3c9dcf467964f2fb82f3a4c63972ee79bf696/layout/printing/nsPrintObject.cpp#94
> I think we should just not set scriptHandlingObject initially when doing a
> static clone.

Is print preview the only thing that clones documents? I'm worried we'll break something else if we change cloning in general so that it doesn't call SetScopeObject. Is that what you meant?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

No, but print/print preview are the only ones to use static clones.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looks like we do read the script handling object during cloning:
http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/dom/base/nsNodeUtils.cpp#455

Let me know what you want me to do.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Make that weaker by allowing cloning to static document without script global

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8831784 [details] [diff] [review]
patch v2

>-  // Set scripting object
>-  bool hasHadScriptObject = true;
Could we always set this when doing static clone, so that we pretend as if the document had had script object.

>@@ -441,16 +441,17 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
>     // documents to "non-scriptable" documents.
>     nsIDocument* newDoc = nodeInfoManager->GetDocument();
>     NS_ENSURE_STATE(newDoc);
>     bool hasHadScriptHandlingObject = false;
>     if (!newDoc->GetScriptHandlingObject(hasHadScriptHandlingObject) &&
>         !hasHadScriptHandlingObject) {
>       nsIDocument* currentDoc = aNode->OwnerDoc();
>       NS_ENSURE_STATE((nsContentUtils::IsChromeDoc(currentDoc) ||
>+                       newDoc->IsStaticDocument() ||
Could we relax this a tiny bit less. Make sure currentDoc's mCreatingStaticClone is true too...but


...hmm, wouldn't it be enough to just have the change to 
nsDocument::CloneDocHelper but keep bool hasHadScriptObject = true; for static clones?
Then change to nsNodeUtils shouldn't be needed since 
(!newDoc->GetScriptHandlingObject(hasHadScriptHandlingObject) && !hasHadScriptHandlingObject)
would be false, right.

Either way should be fine, but I think hasHadScriptObject approach should work and would be simpler.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8831784 [details] [diff] [review]
> patch v2
> 
> >-  // Set scripting object
> >-  bool hasHadScriptObject = true;
> Could we always set this when doing static clone, so that we pretend as if
> the document had had script object.

I'm not sure I understand. hasHadScriptObject is just a local variable that's being read out and used for a check there. Do you want me to set mHasHadScriptHandlingObject to true on the new document but avoid actually giving it a script handling object? That seems really confusing.

> >@@ -441,16 +441,17 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNode, bool aClone, bool aDeep,
> >     // documents to "non-scriptable" documents.
> >     nsIDocument* newDoc = nodeInfoManager->GetDocument();
> >     NS_ENSURE_STATE(newDoc);
> >     bool hasHadScriptHandlingObject = false;
> >     if (!newDoc->GetScriptHandlingObject(hasHadScriptHandlingObject) &&
> >         !hasHadScriptHandlingObject) {
> >       nsIDocument* currentDoc = aNode->OwnerDoc();
> >       NS_ENSURE_STATE((nsContentUtils::IsChromeDoc(currentDoc) ||
> >+                       newDoc->IsStaticDocument() ||
> Could we relax this a tiny bit less. Make sure currentDoc's
> mCreatingStaticClone is true too...but

Sure, that seems fine.

> ...hmm, wouldn't it be enough to just have the change to 
> nsDocument::CloneDocHelper but keep bool hasHadScriptObject = true; for
> static clones?
> Then change to nsNodeUtils shouldn't be needed since 
> (!newDoc->GetScriptHandlingObject(hasHadScriptHandlingObject) &&
> !hasHadScriptHandlingObject)
> would be false, right.

I guess that's true (assuming you mean mHasHadScriptHandlingObject), but again, it seems a bit confusing to me to do it that way. We're changing the meaning of the field.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

bah, I meant the member variable mHasHadScriptHandlingObject.

But the reason is that we do have some checks where if mHasHadScriptHandlingObject returns true but there isn't global anymore, we just don't execute scripts or so, so it would be a bit safer.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v3

Just want to make sure this is what you're asking for.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8832299 [details] [diff] [review]
patch v3

oh, one thing still. We do want NS_ENSURE_STATE(scriptObject || !hasHadScriptObject); check always I think.

So keep
   bool hasHadScriptObject = true;
   nsIScriptGlobalObject* scriptObject =
     GetScriptHandlingObject(hasHadScriptObject);
   NS_ENSURE_STATE(scriptObject || !hasHadScriptObject);
where it is now and then 
if (mCreatingStaticClone) {
  ..
} else if (scriptObject) {
  clone->SetScriptHandlingObject(scriptObject);
else {
  clone->SetScopeObject(GetScopeObject());
}

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: content script patch

This fixes a much simpler problem with print preview. Currently the docshell print preview code is called based on a frame message manager message. The runnables for these messages are automatically labeled with the TabGroup of the frame they're sent to. But the print preview code needs to touch the frame it's cloning the document into as well. I fixed this by moving the actual printPreview call into its own unlabeled runnable.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2cba98ff40
Move print preview code in content script to separate runnable (r=smaug)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6656113311a6
DocGroup should be correct for static clone documents (print, print preview) (r=smaug)

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a2cba98ff40
https://hg.mozilla.org/mozilla-central/rev/6656113311a6