1428860 - XULDocument::GetElementsByAttributeNS may leak memory

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[jbonnafo]

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 1

[jbonnafo]

Attachment: Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

Review commit: https://reviewboard.mozilla.org/r/211286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211286/

Comment 2

[Peter Van der Beken [:peterv]]

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.

Comment 3

[jbonnafo]

Thanks, i will try to implement this.

Comment 4

[jbonnafo]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211286/diff/1-2/

Comment 5

[Peter Van der Beken [:peterv]]

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

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 7

[jbonnafo]

Hello,

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

Thanks,

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/721e8ff721d8
XULDocument::GetElementsByAttributeNS may leak memory r=peterv

Comment 9

[Andreea Pavel [:apavel]]

Backed out for build bustages at /builds/worker/workspace/build/src/dom/xul/XULDocument.cpp 

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=721e8ff721d8b0c81af2c91e3fb660a8cb0be058&

Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=157623513&repo=autoland&lineNumber=19133

Backout: https://hg.mozilla.org/integration/autoland/rev/d5c253aa5fb326756cc03ba2ae3d75226b0a1648

Comment 10

[jbonnafo]

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,

Comment 11

[jbonnafo]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211286/diff/2-3/

Comment 12

[jbonnafo]

Hello,

i have submitted a new fix proposal.

Thanks,

Comment 13

[jbonnafo]

Hello,

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

Thanks,

Comment 14

[Peter Van der Beken [:peterv]]

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.

Comment 15

[jbonnafo]

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,

Comment 16

[Peter Van der Beken [:peterv]]

(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);
                                     ^~~~~~~~~~~~~~~~~~~~

Comment 17

[jbonnafo]

Attachment: Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

Review commit: https://reviewboard.mozilla.org/r/222068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222068/

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 8952843 [details]
Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

https://reviewboard.mozilla.org/r/222068/#review228406

Comment 19

[Mozilla Autoland]

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)

Comment 20

[jbonnafo]

Attachment: Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

Review commit: https://reviewboard.mozilla.org/r/223004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223004/

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 8953819 [details]
Bug 1428860 - XULDocument::GetElementsByAttributeNS may leak memory

https://reviewboard.mozilla.org/r/223004/#review229862

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7431e41b5d95
XULDocument::GetElementsByAttributeNS may leak memory r=peterv

Comment 23

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/7431e41b5d95