Closed Bug 1158540 Opened 6 years ago Closed 6 years ago

Don't repeat the mRefCnt member of URLValue in ImageValue

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Even after this patch, it's not OK to AddRef an ImageValue and then call
Release on its base pointer (URLValue) since URLValue refcounting methods
are not virtual, so it would confuse the leak checker, but at least it
wouldn't cause UAF issues since we'd still be looking at the same mRefCnt
member.
Assignee: nobody → ehsan
Blocks: shadow-field
Comment on attachment 8597641 [details] [diff] [review]
Don't repeat the mRefCnt member of URLValue in ImageValue

>-  // Override AddRef and Release to not only log ourselves correctly, but
>-  // also so that we delete correctly without a virtual destructor
>-  NS_INLINE_DECL_REFCOUNTING(ImageValue)
>+  // Override AddRef and Release to log ourselves correctly.
>+  NS_METHOD_(MozExternalRefCountType) AddRef();
>+  NS_METHOD_(MozExternalRefCountType) Release();

Seems like you should leave the comment as it was, no?

r=dbaron with that
Attachment #8597641 - Flags: review?(roc) → review+
Component: Layout → CSS Parsing and Computation
No, that part of the comment is bogus.  Since the Release method is not virtual in the base class, if you end up destroying an ImageValue through URLValue::Release, you'd end up calling the wrong destructor anyway.  These overrides don't do anything to prevent against that.
But the comment is saying that the reason the override is needed is that if the override were *not* present, there would be no way to release the ImageValue that would call ~ImageValue.
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #4)
> But the comment is saying that the reason the override is needed is that if
> the override were *not* present, there would be no way to release the
> ImageValue that would call ~ImageValue.

Yes, but it also suggests that with the current situation, we would always free ImageValue with calling ~ImageValue, which is not true.  Do you have a better suggestion for the wording of the comment?  I would be happy to fix it.

(FWIW I think the bigger problem with ImageValue is that its refcounting is completely broken because of the above issue, but I can't think of an easy way to fix it, and I have no idea how real the assumption that there's no URLValue* in the program which points to an ImageValue object is.  But fixing this is well beyond the scope of this bug, I think.)
Flags: needinfo?(ehsan) → needinfo?(dbaron)
"delete correctly" -> "delete correctly (assuming callers always call *our* Release method and not our base class's)"


The simple fix for what you're worried about would be to have a URLValueBase that has all of URLValue except for the reference counting bits.
Flags: needinfo?(dbaron) → needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/c112d4cd63e0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(In reply to Pulsebot from comment #9)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bada9c3efe

Improved the comment: ^
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.