If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 54

Status

()

Core
Document Navigation
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: mccr8, Assigned: mystor)

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)

(Reporter)

Description

8 months ago
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

8 months ago
Flags: needinfo?(michael)
(Reporter)

Updated

8 months 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

8 months 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

8 months 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

8 months 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

8 months ago
Assignee: nobody → michael

Updated

8 months ago
Attachment #8836244 - Flags: review?(bugs) → review+
(Reporter)

Updated

8 months ago
status-firefox53: affected → wontfix

Comment 4

8 months 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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e86672310690
Status: NEW → RESOLVED
Last Resolved: 7 months 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.