291183 - [FIXr]FCKeditor crashes when clicking on "Source" button [@ nsStyleSet::ResolveStyleFor ]

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

My 2005-04-19 trunk build crashes when following the following steps on the url:
- Click in the designmode iframe with the sample text, a blinking cursor should
appear.
- Click on the "Source" button (at the top left in the toolbar)

I've mangaged to crash it with a 2004-08-10 trunk build (although not as easily
as current trunk).
I've not managed to crash it with a 204-08-09 trunk build. 
So this might be a regression from bug 230170.

Bug 285972 is about the same issue, but that bug mentions that the crash happens
also with the 1.7 branch (which I can't see).

A minimal testcase is kind of hard, since it is really a lot of js code.

Comment 1

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050419
Firefox/1.0+

Confirmed

talkback: TB5234381G

Comment 2

[Peter van der Woude [:Peter6]]

@ nsStyleSet::ResolveStyleFor 5b6e96c4
no other similar traces found on talkback

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the basic problem here is that we're holding a weakptr to a presshell and we
end up getting its style set after the presshell has been Destroy()ed and before
it's actually gone away.  Then we try to resolve style on the style set (which
has been destroyed), and that tries to dereference its null mRuleTree and things
crash.

So what does the "no prefix getter" guarantee really mean?  nsIPresShell always
give you a style set, but if you're in destruction calling methods on it will
crash.  Perhaps it would be clearer to return null in destruction and fix
callers to check appropriately?  Or perhaps the style set should null-check
mRuleTree everywhere it uses it?

Apart from that, it's not really clear to me why computed style is caching the
_presshell_. It wants to cache the relevant "view", but caching a presshell
means that changing display of an iframe makes computed style for things inside
the iframe die (testcase coming up).  Perhaps computed style should simply
assume it's for the 0th presshell of the ownerDocument of the target node for now?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows exceptions

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Same testcase, but without flushes -- crashes

This is basically a minimal testcase for this bug...  We're flushing reflow,
which destroys the presshell we're holding on to, but it's not quite dead yet
(because we're in event dispatch, or something) so we can still get its
destroyed style set.

Comment 6

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

(In reply to comment #3)
> So what does the "no prefix getter" guarantee really mean?  nsIPresShell
> always give you a style set, but if you're in destruction calling methods on
> it will crash.  Perhaps it would be clearer to return null in destruction and
> fix callers to check appropriately? 

That sounds best to me.

Comment 7

[Chris]

Reproducable: windows 2000, Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.8b2) Gecko/20050419 Firefox/1.0+


Error: some is not defined
Source File:
file:///C:/Documents%20and%20Settings/Administrator/Desktop/testcase.html
Line: 4

Comment 8

[Boris Zbarsky [:bzbarsky]]

Chris, that comment doesn't seem to have any thing to do with this bug.  I'm
assuming you commented on the wrong bug; you may want to find the one you meant
to comment on and do so...

Comment 9

[Chris]

(In reply to comment #8)
> Chris, that comment doesn't seem to have any thing to do with this bug.  I'm
> assuming you commented on the wrong bug; you may want to find the one you meant
> to comment on and do so...

i believe that it does..the only reason i posted a response is because of the
error that was in javascript console....no other reply had posted this
particular error.

javascript console:
Error: some is not defined
Source File:
file:///C:/Documents%20and%20Settings/Administrator/Desktop/testcase.html
Line: 4

Comment 10

[Chris]

(In reply to comment #9)
> (In reply to comment #8)
> > Chris, that comment doesn't seem to have any thing to do with this bug.  I'm
> > assuming you commented on the wrong bug; you may want to find the one you meant
> > to comment on and do so...
> 
> i believe that it does..the only reason i posted a response is because of the
> error that was in javascript console....no other reply had posted this
> particular error.
> 
> javascript console:
> Error: some is not defined
> Source File:
> file:///C:/Documents%20and%20Settings/Administrator/Desktop/testcase.html
> Line: 4

I apologize...i WAS able to reproduce this bug in 20050419 Firefox/1.0+, but the
javascript console error WAS for another bug...again, I apologize.

Comment 11

[Caleb]

Requesting 1.8b3 blocking for this crasher.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

The nsStyleSet changes are not strictly needed to fix this, but are good
policy, imo.  I'd be OK with taking them out, though.

Comment 13

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

Comment on attachment 185188 [details] [diff] [review]
Patch

Unless there's a good reason for us to want to allow the Resolve* calls after
shutdown, could you make the mInShutown tests in nsStyleSet use
NS_ENSURE_FALSE?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185188 [details] [diff] [review]
Patch

Requesting 1.8b3 approval for crash fix.

Comment 15

[chris hofmann]

Comment on attachment 185188 [details] [diff] [review]
Patch

a=chofmann

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8b3

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2005-06-03-06 of Seamonkey trunk on Windows XP
against the following URLs:

http://www.fckeditor.net/Demo/
http://www.fckeditor.net/Demo/Demo02.html
http://www.fckeditor.net/Demo/Demo03.html
http://www.fckeditor.net/Demo/Demo04.html

No crashes.

Comment 18

[FredCK]

Hello Guys... congratulations for your work.

Just to be sure the bug is fixed, please try the following page with two 
instances of the editor:
http://www.fckeditor.net/fckeditor/_samples/html/sample09.html

Switch to Source and back again to WYSIWYG on both editor... the actual 
behavior: first editor switched works, the other one crashes.

FredCK

Comment 19

[Boris Zbarsky [:bzbarsky]]

That testcase worksforme with a current nightly.

Comment 20

[Martijn Wargers (dead)]

Well, it crashed for me once, with a 2005-06-06 build (no talkback, sorry), but
not anymore afterwards.