331117 - Layout module should not shut down until all documents have been released

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Benjamin Smedberg]

The layout module currently shuts down services like nsContentUtils during the "xpcom-shutdown" phase, but it is possible to have documents outstanding after that point, due to module-shutdown sequencing and GC.

I want to keep a "module refcount" of outstanding document objects that would delay layout shutdown.

Comment 1

[Benjamin Smedberg]

Attachment: Layout shutdown refcountuing, rev. 1

Comment 2

[Benjamin Smedberg]

I'm looking at perhaps backporting this to branches to solve issues like bug 237736.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the idea here is that this patch doesn't change anything but you'll do that in a later patch?

Comment 4

[Benjamin Smedberg]

Attachment: Actually use it

That was my original intent: but this is the only consumer I need at this time.

Comment 5

[Benjamin Smedberg]

Attachment: Layout shutdown refcountuing, rev. 1.1

Update to trunk, and this also refcounts in globalwindow impls.

Comment 6

[Mark Mentovai]

Attachment: v1.1 updated to current trunk

Comment 7

[Mark Mentovai]

Comment on attachment 222092 [details] [diff] [review]
v1.1 updated to current trunk

This patch addresses the startup crash reported in bug 337481.

Comment 8

[Mike Shaver (:shaver emeritus)]

Raising severity here, because this seems to be the appropriate way to resolve 337481, which very much blocks testing on OS X right now.

Comment 9

[Mike Shaver (:shaver emeritus)]

(Hunk 6 of nsLayoutModule.cpp's patch here conflicts for me now, likely easy to solve.)

Comment 10

[Benjamin Smedberg]

I'm not going to remake this patch until I get agreement from dbaron on the general premise that I'm taking. 

Comment 11

[Darin Fisher]

The code inserted into dom/src/base/nsGlobalWindow.h bumps the comment and causes it to lose context.  Not intended, right?

Comment 12

[Darin Fisher]

Another solution to this bug might be to make Layout (and content) cancel all network requests and other asynchronous operations during shutdown.  That's typically how most consumers of necko behave.  Of course that doesn't help if someone else holds onto a document post xpcom-shutdown.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 217859 [details] [diff] [review]
Layout shutdown refcountuing, rev. 1.1

I suppose with comment 11 fixed this is ok with me.  Although I'm a little concerned that nsAutoLayoutStatics might add to the size of the objects that use it -- have you tested whether it does so?  It might be better to just add to constructor/destructor.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Darin, the problem is that canceling the network connection at xpcom-shutdown is too late, since the OnStopRequest happens async sometime after that.  Hence my suggestion for a separate phase in shutdown for canceling anything async, with event processing after to make sure the notifications trigger...

Comment 15

[Benjamin Smedberg]

Shutting down network requests early doesn't help in general, only in some specific cases.

Fixed on trunk, with the comment-move, and nsAutoLayoutStatics removed (it takes up a byte per-instance, because C++ pointer-to-members require it).

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Shutting down network requests early doesn't help in general, only in some
> specific cases.

Sure.  As in, all specific cases when anyone has a network connection open at shutdown.  As in, I feel we should do the network thing in addition to this patch.

Comment 17

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060519 Minefield/3.0a1 ID:2006051905 [cairo]

is this check-in cause below?

on Exit/shutdown Minefield.
http://img253.imageshack.us/img253/7456/clear2og.jpg

History>Clear Private Data
this window is fine.

Comment 18

[Benjamin Smedberg]

Attachment: Merged to 1.8 branch

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 229153 [details] [diff] [review]
Merged to 1.8 branch

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and marked fixed1.8.1 when you have done so.

Comment 20

[Benjamin Smedberg]

Fixed on 1.8 branch.