Last Comment Bug 714162 - Don't traverse certainly alive selections
: Don't traverse certainly alive selections
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: x86_64 Linux
-- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug]
: Jet Villegas (:jet)
Depends on:
Blocks: 716598 640853 728705
  Show dependency treegraph
Reported: 2011-12-29 12:51 PST by Olli Pettay [:smaug]
Modified: 2012-03-14 00:42 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (1.08 KB, patch)
2011-12-29 12:51 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review
Safer version (1.68 KB, patch)
2012-01-04 12:58 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review

Description User image Olli Pettay [:smaug] 2011-12-29 12:51:22 PST
Created attachment 584818 [details] [diff] [review]

nsFrameSelection is kept alive by Presshell (until Presshell is deleted) and
for text form controls nsTextEditorState/nsTextControlFrame.
When presshell is going away mShell is set to null using DisconnectFromPresShell.

So, as long a mShell is there, the nsFrameSelection object should be certainly alive and 
the whole traversing is just useless.
Comment 1 User image Mats Palmgren (:mats) 2011-12-29 13:52:55 PST
Yes, the above comment is correct.  I'm not familiar with the cycle collector or
what NS_SUCCESS_INTERRUPTED_TRAVERSE means though, so you should get someone else
to review the actual code change.
Comment 2 User image Olli Pettay [:smaug] 2011-12-29 14:05:28 PST
Comment 3 User image Andrew McCreight [:mccr8] 2011-12-29 16:23:47 PST
Comment on attachment 584818 [details] [diff] [review]

Review of attachment 584818 [details] [diff] [review]:

Looks good. Death to death stars!

r=me given mats's comment.
Comment 4 User image Olli Pettay [:smaug] 2011-12-30 03:12:00 PST
Comment 5 User image Olli Pettay [:smaug] 2012-01-04 12:58:22 PST
Created attachment 585860 [details] [diff] [review]
Safer version

I started to worry about this a bit, so better be safe.
Comment 6 User image Andrew McCreight [:mccr8] 2012-01-05 08:28:43 PST
Comment on attachment 585860 [details] [diff] [review]
Safer version

Review of attachment 585860 [details] [diff] [review]:

The diff on top of the previous patch wasn't really necessary for a tiny patch.

At some point, we should probably another CC macro for this pattern (if document is non-null and in the current generation, return interrupted traverse).
Comment 7 User image Olli Pettay [:smaug] 2012-01-06 11:18:41 PST

Note You need to log in before you can comment on or make changes to this bug.