Closed Bug 1428860 Opened 6 years ago Closed 5 years ago

XULDocument::GetElementsByAttributeNS may leak memory

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: jeanluc.bonnafoux, Assigned: jeanluc.bonnafoux)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory
59 bytes, text/x-review-board-request
peterv
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

Static C++ analysis of code (cppcheck) points out that XULDocument::GetElementsByAttributeNS may have a memory leak


Actual results:

This happens only if the call to RegisterNameSpace returns an error.
in that case the local variable attrValue is not deleted accordingly


Expected results:

If RegisterNameSpace returns an error, attrValue  should be deleted accordingly.
Comment on attachment 8941003 [details]
Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

https://reviewboard.mozilla.org/r/211286/#review218114

::: dom/xul/XULDocument.cpp:1250
(Diff revision 1)
>        nsresult rv =
>          nsContentUtils::NameSpaceManager()->RegisterNameSpace(aNamespaceURI,
>                                                                nameSpaceId);
>        if (NS_FAILED(rv)) {
>            aRv.Throw(rv);
> +          delete attrValue;

Make attrValue a nsAutoPtr<nsString> (or UniquePtr<nsString>) instead, and .forget() where it's passed to the nsContentList constructor.
Attachment #8941003 - Flags: review?(peterv) → review-
Thanks, i will try to implement this.
Comment on attachment 8941003 [details]
Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

https://reviewboard.mozilla.org/r/211286/#review219692
Attachment #8941003 - Flags: review?(peterv) → review+
Assignee: nobody → jeanluc.bonnafoux
Status: UNCONFIRMED → ASSIGNED
status-firefox58: --- → wontfix
status-firefox59: --- → fix-optional
Ever confirmed: true
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(jeanluc.bonnafoux)
Keywords: checkin-needed
Hello,

I have amended the mozreview page accordingly.
I hope it will be OK now.

Thanks,
Flags: needinfo?(jeanluc.bonnafoux)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/721e8ff721d8
XULDocument::GetElementsByAttributeNS may leak memory r=peterv
Keywords: checkin-needed
Hello,

Thanks for the info, understood from logs this code does not compile with g++ whereas VC++ 2017 is ok.
I am going to work on a new patch proposal that uses reinterpret_cast to instruct to compiler to convert to void*

Thanks,
Flags: needinfo?(jeanluc.bonnafoux)
Hello,

i have submitted a new fix proposal.

Thanks,
Attachment #8941003 - Flags: review+ → review?(peterv)
Hello,

Could you please tell me if the new patch proposal has been submitted ok in mozreview?

Thanks,
Comment on attachment 8941003 [details]
Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

https://reviewboard.mozilla.org/r/211286/#review224774

::: dom/xul/XULDocument.cpp:1119
(Diff revision 3)
>  already_AddRefed<nsINodeList>
>  XULDocument::GetElementsByAttribute(const nsAString& aAttribute,
>                                      const nsAString& aValue)
>  {
>      RefPtr<nsAtom> attrAtom(NS_Atomize(aAttribute));
> -    void* attrValue = new nsString(aValue);
> +    nsAutoPtr<nsString> attrValue = new nsString(aValue);

The compile error is on this line, you probably need to do

nsAutoPtr<nsString> attrValue(new nsString(aValue));

::: dom/xul/XULDocument.cpp:1123
(Diff revision 3)
>      RefPtr<nsAtom> attrAtom(NS_Atomize(aAttribute));
> -    void* attrValue = new nsString(aValue);
> +    nsAutoPtr<nsString> attrValue = new nsString(aValue);
>      RefPtr<nsContentList> list = new nsContentList(this,
>                                              MatchAttribute,
>                                              nsContentUtils::DestroyMatchString,
> -                                            attrValue,
> +                                            reinterpret_cast<void*>(attrValue.forget()),

Remove the reinterpret_cast.
Attachment #8941003 - Flags: review?(peterv) → review-
Thanks for the review.

Code without the reinterpret_cast failed to compile on Linux (Comment 9).
This is why i have added this cast operation.
I am going to amend object construction following your comment but i think the reinterpret_cast may be needed.

Thanks,
(In reply to jbonnafo from comment #15)
> Code without the reinterpret_cast failed to compile on Linux (Comment 9).
> This is why i have added this cast operation.

The error in the log is:

/builds/worker/workspace/build/src/dom/xul/XULDocument.cpp:1149:37: error: conversion from 'nsString* {aka nsTString<char16_t>*}' to non-scalar type 'nsAutoPtr<nsTString<char16_t> >' requested
     nsAutoPtr<nsString> attrValue = new nsString(aValue);
                                     ^~~~~~~~~~~~~~~~~~~~
Attachment #8941003 - Attachment is obsolete: true
Comment on attachment 8952843 [details]
Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

https://reviewboard.mozilla.org/r/222068/#review228406
Attachment #8952843 - Flags: review?(peterv) → review+
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d34220b17ede9078025bfa9a2c503efb3a7044f4 -d 3b82dc57cc73: rebasing 448640:d34220b17ede "Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory r=peterv" (tip)
merging dom/xul/XULDocument.cpp
warning: conflicts while merging dom/xul/XULDocument.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Attachment #8952843 - Attachment is obsolete: true
Comment on attachment 8953819 [details]
Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

https://reviewboard.mozilla.org/r/223004/#review229862
Attachment #8953819 - Flags: review?(peterv) → review+
Keywords: checkin-needed
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7431e41b5d95
XULDocument::GetElementsByAttributeNS may leak memory r=peterv
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7431e41b5d95
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.