593552 - Patch for bug 587064 is wrong

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

It makes us get the refresh driver via the docshell, but docshells are shared across multiple documents, so we can end up getting it from the wrong document.

On the testcase in that bug, we have:

* mDocument's URI is "http://www.youtube.com/js/pyv_watch_request_ad.html"
* parent's URI is about:blank
* parent has no presshell
* mDocument's container's parent is parent's container
* mDocument's container's document is mDocument
* mDocument's container's parent's current document is NOT parent
* mDocument is a static document
* parent is NOT a static document
* mDocument's container's parent's current document is a static document
* mDocument's container's parent's current document's URI is
  "http://www.youtube.com/watch?v=r_GlsoXt70M"

I have no idea what the print engine is doing here with subframes and where that about:blank thing came from and why it's not a static document, but it's pretty messed up...

More importantly, if this code runs during a page transition, it may be that the parent docshell's current document is no longer our real parent document, and then we'll just use totally the wrong refresh driver.

I think the right fix is to back out bug 587064 and just add a null-check: if the parent has no presshell, fall back to creating our own refresh driver.

Olli, any idea where this about:blank thing came from?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Er, and even ccing smaug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, |parent| is really an about:blank document  It has an empty head and an empty body.

The only reason it's anyone's parent is that in DocumentViewerImpl::SyncParentSubDocMap we end up with the about:blank document as parent_doc and a |content| which is in a totally different doc.  So the docshells are hooked up one way (apparently to this about:blank document) but the actual nodes doing the loads are in totally different places.  That seems Really Broken.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And in particular, the owner document of |content| there is a static document.  parent_doc, of course, is not.

Comment 4

[Boris Zbarsky [:bzbarsky]]

We should also get a bug filed on this, which happens with the bug 587064 STR:

###!!! ASSERTION: Unexpected containers: 'SameCOMIdentity(debugDocContainer, debugDocShell)', file ../../../mozilla/layout/base/nsDocumentViewer.cpp, line 2165

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 6

[Timothy Nikkel (:tnikkel)]

The "Unexpected containers" assert happens when printing pretty much anything.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ok, but that doesn't mean we shouldn't fix it!

Comment 8

[Timothy Nikkel (:tnikkel)]

For sure. Just saying you don't need to do anything special to see it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 472120 [details] [diff] [review]
Fix

Safe fix; makes sure we don't share refresh drivers when maybe we shouldn't.

Comment 10

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

Yeah, we should just remove the assertion.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/f5206ff5dce8