129620 - theme switch can cause nsDocument to be asked to remove same stylesheet twice

Status: RESOLVED INCOMPLETE

(Core :: CSS Parsing and Computation, defect, P5)

Description

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

I've been tracking down a skin-switching crash that I've managed to figure out a
way to reproduce about half the time.  This may be related to bug 127716.  The
steps to reproduce are roughly the following:
 * start the browser (initially in Modern)
 * pres control-two to get into mail
 * cancel at the password dialog
 * click on my news server in the folder pane
 * click on one of the news groups under the server
 * scroll the thread pane using both the slider thumb and the arrows
 * click on one of the articles in the thread pane, so that it shows up
 * ctrl-click on the article above/below that article, so no article is displaying
 * in the browser window, click View | Apply Theme | Classic
 * switch back to the mail window, and click on another article, so that that
article is displaying
 * focus (raise) the browser window
 * click View | Apply Theme | Modern
 * focus the mail window
 * click the X in the corner of the mail window to close the mail window
 * click the X in the corner of the browser window to close the browser window

At some point near the end of this chain, it crashes.  (I actually stubbed out
CSSStyleSheetImpl::Release so it doesn't crash but I can still get debugging
info for the refcount log.)  I took a refcount log where everything on the
over-released sheet is balanced except for nsDocument's handling of stylesheets.
(Note that the refcounting described in bug 129618 is rather bizarre.)

So at the lowest level the problem seems to be that nsDocument doesn't check the
return value when it calls mStyleSheets.RemoveElement(), and then proceeds to
release the sheets as if it owns a reference.  If the sheet isn't in
mStyleSheets (I've yet to figure out why *this* is the case beyond that
mStyleSheets.RemoveElement() is being called from both callsites in
nsDocument.cpp for a single stylesheet) this can end up over-releasing the
stylesheets.

I'll attach the excerpts from the refcount log (in order) and the balance tree
showing the problem.

Comment 1

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

Attachment: refcount log for over-released stylesheet

Comment 2

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

Attachment: balance tree for over-released stylesheet

Comment 3

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

Comment on attachment 73129 [details]
refcount log for over-released stylesheet

It should be clear in this log that the final "Release 6" and the final
"Release 2" both correspnd to the second "AddRef 7".

Comment 4

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

Attachment: bulletproofing patch (diff -u)

Comment 5

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

Attachment: bulletproofing patch (diff -uw, for reading but not applying)

Comment 6

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

I'm wondering if this is happening because mail inserts a LINK element into the
email message being displayed or something like that.  I don't see why a HTML
document would link to a chrome stylesheet, and I don't think the ChromeRegistry
code should be doing anything to non-chrome stylesheets, although I could be
wrong.  It seems like the problem that causes is that the style link element
code (nsHTMLLinkElement) doesn't know that the link it represents now maps to a
different stylesheet object.  (Perhaps the same problem could happen for PIs in
XUL?)

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 73136 [details] [diff] [review]
bulletproofing patch (diff -uw, for reading but not applying)

sr=jst

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 73135 [details] [diff] [review]
bulletproofing patch (diff -u)

r=peterv, but it'd be good to figure out why this happens exactly. Should we
leave the bug open?

Comment 9

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

Comment on attachment 73135 [details] [diff] [review]
bulletproofing patch (diff -u)

So that this will compile on Windows I need to write the first change as:

-  mStyleSheets.RemoveElement(aSheet);
+  if (!mStyleSheets.RemoveElement(aSheet)) {
+    NS_NOTREACHED("stylesheet not found");
+    return;
+  }

(Why does gcc only warn about returning something in a void function rather
than giving an error?)

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 73136 [details] [diff] [review]
bulletproofing patch (diff -uw, for reading but not applying)

a=asa (on behalf of drivers) for checkin to the 0.9.9 branch and the 1.0 trunk

Comment 11

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

OK, that patch was checked in to:
 * trunk, 2002-03-08 13:05 PST
 * MOZILLA_0_9_9_BRANCH, 2002-03-08 13:09 PST.

Retitling this bug to cover the remaining problem and pushing it off to mozilla1.1.

Comment 12

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

*** Bug 122111 has been marked as a duplicate of this bug. ***

Comment 13

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

*** Bug 127716 has been marked as a duplicate of this bug. ***

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 73136 [details] [diff] [review]
bulletproofing patch (diff -uw, for reading but not applying)

obsoleting checked in patche.

Comment 15

[Alfred Kayser]

Marking as fixed, as the patch has been applied (7 years ago), and this issue (crash caused by removing an non-existing stylesheet) is confirmed fixed by that patch.

Comment 16

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

Reopening per comment 11.

Comment 17

[Andrei Purice]

Hey David,
Can you still reproduce this or should we close it?

Comment 18

[Andrei Purice]

Marking this as Resolved > Incomplete since the last activity on this issue was 12 years ago and it might not be relevant anymore.
Feel free to re-open if the issue is still reproducible on your end in the latest FF versions.