473773 - DocumentViewerImpl::InitPresentationStuff can reenter

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Peter Van der Beken [:peterv]]

I ran into this while debugging a crash in bug 471126. When loading the crashtest 458637-1.html I sometimes see us reentering DocumentViewerImpl::InitPresentationStuff. I've attached a stack that shows what happens. The testcase is a document with an iframe. The iframe contains a document with an embedded XSLT stylesheet that has an error. We end up in DocumentViewerImpl::SetDOMDocument to set the document to the error document, while there's still a pending restyle on the frame. DocumentViewerImpl::InitPresentationStuff calls mViewManager->EnableRefresh(NS_VMREFRESH_IMMEDIATE) which ends up dealing with the pending restyle, calling DocumentViewerImpl::Show which calls DocumentViewerImpl::InitPresentationStuff again.

There's a couple of ways to fix this, but it's hard for me to judge what would be the right thing to do. Boris suggested passing aReenableRefresh=PR_FALSE to InitPresentationStuff from SetDOMDocument and calling EnableRefresh from SetDOMDocument after InitPresentationStuff has returned. We can also reorder InitPresentationStuff so that the EnableRefresh call is the last thing and reentering is safer. Boris was worried about potential flickering. Roc, any opinions?

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Stack trace

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Stack trace

Comment 3

[Martijn Wargers (dead)]

Related to bug 470582?

Comment 4

[Boris Zbarsky [:bzbarsky]]

To be clear, the major flicker worry was if we change from NS_VMREFRESH_IMMEDIATE to NS_VMREFRESH_DEFERRED in this code.  Moving the EnableRefresh should be reasonably safe, I think, but I wanted roc to sanity-check that.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Passing aReenableRefresh=PR_FALSE sounds good to me.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 7

[Martijn Wargers (dead)]

(In reply to comment #3)
> Related to bug 470582?

Ok, never mind, bug 470582 still seems to crash with the 2009-01-19 build.

Comment 8

[Johnny Stenback (:jst)]

Blocking since this blocks blocker bug 471126.