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

RESOLVED FIXED in mozilla1.8final

Status

()

Core
Layout
P1
blocker
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

({fixed1.8.1})

Trunk
mozilla1.8final
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 215808 [details] [diff] [review]
Layout shutdown refcountuing, rev. 1
Attachment #215808 - Flags: superreview?(dbaron)
Attachment #215808 - Flags: review?(dbaron)
(Assignee)

Updated

12 years ago
Blocks: 237736
(Assignee)

Comment 2

12 years ago
I'm looking at perhaps backporting this to branches to solve issues like bug 237736.
Target Milestone: mozilla1.9alpha → mozilla1.8final
(Assignee)

Updated

12 years ago
Priority: -- → P1
So the idea here is that this patch doesn't change anything but you'll do that in a later patch?
(Assignee)

Comment 4

12 years ago
Created attachment 217710 [details] [diff] [review]
Actually use it

That was my original intent: but this is the only consumer I need at this time.
Attachment #217710 - Flags: superreview?(dbaron)
Attachment #217710 - Flags: review?(dbaron)
(Assignee)

Comment 5

12 years ago
Created attachment 217859 [details] [diff] [review]
Layout shutdown refcountuing, rev. 1.1

Update to trunk, and this also refcounts in globalwindow impls.
Attachment #215808 - Attachment is obsolete: true
Attachment #217710 - Attachment is obsolete: true
Attachment #217859 - Flags: superreview?(dbaron)
Attachment #217859 - Flags: review?(dbaron)
Attachment #215808 - Flags: superreview?(dbaron)
Attachment #215808 - Flags: review?(dbaron)
Attachment #217710 - Flags: superreview?(dbaron)
Attachment #217710 - Flags: review?(dbaron)

Comment 6

12 years ago
Created attachment 222092 [details] [diff] [review]
v1.1 updated to current trunk

Comment 7

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

This patch addresses the startup crash reported in bug 337481.
Raising severity here, because this seems to be the appropriate way to resolve 337481, which very much blocks testing on OS X right now.
Severity: normal → blocker
(Hunk 6 of nsLayoutModule.cpp's patch here conflicts for me now, likely easy to solve.)
(Assignee)

Comment 10

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

Comment 11

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

Comment 12

12 years ago
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 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.
Attachment #217859 - Flags: superreview?(dbaron)
Attachment #217859 - Flags: superreview+
Attachment #217859 - Flags: review?(dbaron)
Attachment #217859 - Flags: review+
Summary: Layout module should not shut down until all documents have been release → Layout module should not shut down until all documents have been released
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...
(Assignee)

Comment 15

12 years ago
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).
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
> 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

12 years ago
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.
(Assignee)

Comment 18

12 years ago
Created attachment 229153 [details] [diff] [review]
Merged to 1.8 branch
Attachment #229153 - Flags: review?(dbaron)
Attachment #229153 - Flags: approval1.8.1?
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.
Attachment #229153 - Flags: review?(dbaron)
Attachment #229153 - Flags: review+
Attachment #229153 - Flags: approval1.8.1?
Attachment #229153 - Flags: approval1.8.1+
(Assignee)

Comment 20

12 years ago
Fixed on 1.8 branch.
Keywords: fixed1.8.1
Blocks: 345157
You need to log in before you can comment on or make changes to this bug.