Closed Bug 593552 Opened 9 years ago Closed 9 years ago

Patch for bug 587064 is wrong

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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?
Er, and even ccing smaug.
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.
And in particular, the owner document of |content| there is a static document.  parent_doc, of course, is not.
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
Attached patch FixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #472120 - Flags: review?(roc)
blocking2.0: --- → ?
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Whiteboard: [need review]
The "Unexpected containers" assert happens when printing pretty much anything.
Ok, but that doesn't mean we shouldn't fix it!
For sure. Just saying you don't need to do anything special to see it.
Whiteboard: [need review] → [need approval]
Comment on attachment 472120 [details] [diff] [review]
Fix

Safe fix; makes sure we don't share refresh drivers when maybe we shouldn't.
Attachment #472120 - Flags: approval2.0?
Attachment #472120 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Yeah, we should just remove the assertion.
Pushed http://hg.mozilla.org/mozilla-central/rev/f5206ff5dce8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b6
You need to log in before you can comment on or make changes to this bug.