Closed Bug 1251235 Opened 4 years ago Closed 4 years ago

[Static Analysis][Resource leak] In function nsHtml5ViewSourceUtils::NewBodyAttributes

Categories

(Core :: HTML: Parser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1354263 btpp-active)

Attachments

(1 file)

The Static Analysis tool Coverity added that |klass| can leak memory, this can happen only these two ifs are false:

>>  if (mozilla::Preferences::GetBool("view_source.wrap_long_lines", true)) {
>>    klass->Append(NS_LITERAL_STRING("wrap "));
>>  }
>>  if (mozilla::Preferences::GetBool("view_source.syntax_highlight", true)) {
>>    klass->Append(NS_LITERAL_STRING("highlight"));
>>  }
Comment on attachment 8723559 [details]
MozReview Request: Bug 1251235 - changed from naked pointers to UniquePtr to prevent resource leak. r?froydnj

I am not a reviewer for this module, but it seems :hsivonen is out.  Let's try... :froydnj looking at commit logs.
Attachment #8723559 - Flags: review?(jryans) → review?(nfroyd)
Attachment #8723559 - Flags: review?(nfroyd)
Comment on attachment 8723559 [details]
MozReview Request: Bug 1251235 - changed from naked pointers to UniquePtr to prevent resource leak. r?froydnj

https://reviewboard.mozilla.org/r/36607/#review33451

I'm not a peer, but as this just involves Gecko-ish C++ issues, I think I'm OK to review it.  (I imagine jryans thinking I could review this file comes from refactoring patches, not from substantial changes ;)

::: parser/html/nsHtml5ViewSourceUtils.cpp:18
(Diff revision 1)
>    nsString* klass = new nsString();

Please change this to:

auto klass = MakeUnique<nsString>();

You probably need to #include "mozilla/UniquePtr.h" above.

::: parser/html/nsHtml5ViewSourceUtils.cpp:26
(Diff revision 1)
>      bodyAttrs->addAttribute(nsHtml5AttributeName::ATTR_CLASS, klass);

..and you'll need to change this to klass.release().

::: parser/html/nsHtml5ViewSourceUtils.cpp:28
(Diff revision 1)
> +  else {
> +    delete klass;
> +  }

...now you don't need this.
Whiteboard: CID 1354263 → CID 1354263 btpp-active
Comment on attachment 8723559 [details]
MozReview Request: Bug 1251235 - changed from naked pointers to UniquePtr to prevent resource leak. r?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36607/diff/1-2/
Attachment #8723559 - Attachment description: MozReview Request: Bug 1251235 - ensure |klass| doesn't leak memory. r?jryans → MozReview Request: Bug 1251235 - changed from naked pointers to UniquePtr to prevent resource leak. r?froydnj
Attachment #8723559 - Flags: review?(nfroyd)
Hello Nathan, i've also modified the encapsulation for the other pointers hope it's OK.
Comment on attachment 8723559 [details]
MozReview Request: Bug 1251235 - changed from naked pointers to UniquePtr to prevent resource leak. r?froydnj

https://reviewboard.mozilla.org/r/36607/#review33675
Attachment #8723559 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/5bff8ab685ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.