Uninitialized rv in PendingGlobalHistoryEntry::ApplyChanges(IHistory* aHistory)

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: Nika)

Tracking

(Blocks: 1 bug, {csectype-uninitialized, regression})

unspecified
mozilla54
csectype-uninitialized, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox51 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53 wontfix, firefox54 fixed)

Details

(Whiteboard: CID 1399513)

Attachments

(1 attachment)

I found this while looking at Coverity results:

  for (const URITitle& title : mTitles) {
    aHistory->SetURITitle(title.mURI, title.mTitle);
    NS_ENSURE_SUCCESS(rv, rv);
  }
  mTitles.Clear();

The rv check there looks completely bogus. SetURITitle doesn't return an rv, so maybe the check just needs to be removed? I don't know if there are sec implications, so I'm filing this hidden.
(Reporter)

Updated

2 years ago
Flags: needinfo?(michael)
(Reporter)

Updated

2 years ago
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
Keywords: regression
(Assignee)

Comment 1

2 years ago
I highly doubt that this will have any sec implications. 

1. This code is never hit in firefox without enabling the super buggy prerendering pref, which is turned on on no channels currently.
2. This not early returning has almost no impact on the behavior of this function, as mTitles is almost always 1 long.

I'll write up a simple patch anyway. It's not worth it to uplift IMO.
Flags: needinfo?(michael)
(Reporter)

Comment 2

2 years ago
(In reply to Michael Layzell [:mystor] from comment #1)
> I'll write up a simple patch anyway. It's not worth it to uplift IMO.

Thanks. I guess the only question is if it could be causing any of the intermittent failures blocking bug  1330332.
Group: dom-core-security
(Assignee)

Comment 3

2 years ago
Created attachment 8836244 [details] [diff] [review]
Check the return value from aHistory->SetURITitle when applying pending global history entries

I think it is extremely unlikely that this is the cause of any of those intermittent failures. 

MozReview-Commit-ID: 6aPtNrqoqZj
Attachment #8836244 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Assignee: nobody → michael
Attachment #8836244 - Flags: review?(bugs) → review+
(Reporter)

Updated

2 years ago
status-firefox53: affected → wontfix

Comment 4

2 years ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e86672310690
Check the return value from aHistory->SetURITitle when applying pending global history entries, r=smaug

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e86672310690
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.