Last Comment Bug 725446 - BBP for ContentList
: BBP for ContentList
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 13 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla13
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 716598
  Show dependency treegraph
 
Reported: 2012-02-08 12:27 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-02-10 13:17 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (3.96 KB, patch)
2012-02-08 12:27 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
continuation: feedback+
Details | Diff | Review
patch (3.81 KB, patch)
2012-02-10 10:55 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jst: review+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-08 12:27:13 PST
Created attachment 595490 [details] [diff] [review]
WIP
Comment 1 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-02-10 08:28:41 PST
Comment on attachment 595490 [details] [diff] [review]
WIP

Review of attachment 595490 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, aside from my one comment.

::: content/base/src/nsContentList.cpp
@@ +93,5 @@
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsBaseContentList)
> +  if (nsCCUncollectableMarker::sGeneration && tmp->IsBlack()) {
> +    JSObject* o = tmp->PreservingWrapper() ? 
> +      tmp->GetWrapperPreserveColor() : tmp->GetExpandoObjectPreserveColor();
> +    xpc_UnmarkGrayObject(o);

Didn't we just check that our wrapper is black?  Isn't o going to always be non-gray here?
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 08:31:48 PST
Yes. silly me. I'll change the code to unmark only if !tmp->PreservingWrapper()
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 08:32:58 PST
Er, except that is not needed.
Comment 4 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-02-10 08:40:58 PST
Because if the list is owned by a black wrapper, it won't also have a separate expando object that it owns?
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 10:43:58 PST
Well, it is the black proxy object which has the expando. And that should be be black too.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 10:44:47 PST
(Need to get some documentation all this preservedwrapper/proxy/expando handling)
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 10:55:13 PST
Created attachment 596109 [details] [diff] [review]
patch
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 11:08:09 PST
(In reply to Olli Pettay [:smaug] from comment #5)
> Well, it is the black proxy object which has the expando. And that should be
> be black too.
Since if something marks the proxy black, its reserved slots get marked too.
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-10 13:16:13 PST
https://hg.mozilla.org/mozilla-central/rev/445e1040bfcf

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