Note: There are a few cases of duplicates in user autocompletion which are being worked on.

BBP for ContentList

RESOLVED FIXED in mozilla13

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

13 Branch
mozilla13
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 595490 [details] [diff] [review]
WIP
Blocks: 716598
(Assignee)

Updated

6 years ago
Attachment #595490 - Flags: review?(jst)
Attachment #595490 - Flags: feedback?(continuation)
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?
Attachment #595490 - Flags: feedback?(continuation) → feedback+
(Assignee)

Comment 2

6 years ago
Yes. silly me. I'll change the code to unmark only if !tmp->PreservingWrapper()
(Assignee)

Comment 3

6 years ago
Er, except that is not needed.
Because if the list is owned by a black wrapper, it won't also have a separate expando object that it owns?
(Assignee)

Comment 5

6 years ago
Well, it is the black proxy object which has the expando. And that should be be black too.
(Assignee)

Comment 6

6 years ago
(Need to get some documentation all this preservedwrapper/proxy/expando handling)
(Assignee)

Comment 7

6 years ago
Created attachment 596109 [details] [diff] [review]
patch
Assignee: nobody → bugs
Attachment #595490 - Attachment is obsolete: true
Attachment #595490 - Flags: review?(jst)
Attachment #596109 - Flags: review?(jst)
(Assignee)

Comment 8

6 years ago
(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.

Updated

6 years ago
Attachment #596109 - Flags: review?(jst) → review+
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/445e1040bfcf
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla13
Version: 12 Branch → 13 Branch
You need to log in before you can comment on or make changes to this bug.