Closed Bug 291183 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Layout, defect, P1)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.8beta3

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

References

()

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(3 files)

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.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050419
Firefox/1.0+

Confirmed

talkback: TB5234381G
@ nsStyleSet::ResolveStyleFor 5b6e96c4
no other similar traces found on talkback
Summary: FCKeditor crashes when clicking on "Source" button → FCKeditor crashes when clicking on "Source" button [@ nsStyleSet::ResolveStyleFor ]
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?
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.
(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.
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
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...
(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
(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.
Keywords: testcase
Summary: FCKeditor crashes when clicking on "Source" button [@ nsStyleSet::ResolveStyleFor ] → FCKeditor crashes when clicking on "Source" button [@ nsStyleSet::ResolveStyleFor ]
Requesting 1.8b3 blocking for this crasher.
Flags: blocking1.8b3?
Attached patch PatchSplinter Review
The nsStyleSet changes are not strictly needed to fix this, but are good
policy, imo.  I'd be OK with taking them out, though.
Attachment #185188 - Flags: superreview?(dbaron)
Attachment #185188 - Flags: review?(dbaron)
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?
Attachment #185188 - Flags: superreview?(dbaron)
Attachment #185188 - Flags: superreview+
Attachment #185188 - Flags: review?(dbaron)
Attachment #185188 - Flags: review+
Comment on attachment 185188 [details] [diff] [review]
Patch

Requesting 1.8b3 approval for crash fix.
Attachment #185188 - Flags: approval1.8b3?
Assignee: nobody → bzbarsky
Priority: -- → P1
Summary: FCKeditor crashes when clicking on "Source" button [@ nsStyleSet::ResolveStyleFor ] → [FIXr]FCKeditor crashes when clicking on "Source" button [@ nsStyleSet::ResolveStyleFor ]
Target Milestone: --- → mozilla1.8beta3
Comment on attachment 185188 [details] [diff] [review]
Patch

a=chofmann
Attachment #185188 - Flags: approval1.8b3? → approval1.8b3+
Fixed for 1.8b3
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
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
That testcase worksforme with a current nightly.
Well, it crashed for me once, with a 2005-06-06 build (no talkback, sorry), but
not anymore afterwards.
Flags: blocking1.8b3?
Blocks: 230170
Crash Signature: [@ nsStyleSet::ResolveStyleFor ]
You need to log in before you can comment on or make changes to this bug.