Closed Bug 1222754 Opened 9 years ago Closed 8 years ago

Replace nsSecureBrowserUIImpl::mOnStateLocationChangeReentranceDetection and related classes

Categories

(Core :: Security: PSM, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

(David Keeler [:keeler] (use needinfo?) from Bug 1110935 comment #6)
> ::: security/manager/ssl/nsSecureBrowserUIImpl.h:81
> (Diff revision 1)
> >    mozilla::Atomic<int32_t> mOnStateLocationChangeReentranceDetection;
> 
> I believe this doesn't need to be atomic any more (since it's not
> multi-threaded any more). It's also unclear to me that this is necessary at
> all. These are probably changes/investigations for another bug, though.
nsSecureBrowserUIImpl is simultaneously critical to security UI, and a mess.

So, maybe the safer option here is to just replace mOnStateLocationChangeReentranceDetection with mozilla::ReentrancyGuard for now, and remove the guard if/when nsSecureBrowserUIImpl isn't a mess.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Summary: Investigate whether mOnStateLocationChangeReentranceDetection is still necessary → Replace nsSecureBrowserUIImpl::mOnStateLocationChangeReentranceDetection and related classes
Whiteboard: [psm-cleanup] → [psm-assigned]
mOnStateLocationChangeReentranceDetection and nsAutoAtomic form an unnecessarily
threadsafe reentrance prevention mechanism that can be replaced by
mozilla::ReentrancyGuard.

Review commit: https://reviewboard.mozilla.org/r/54160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54160/
Attachment #8754669 - Flags: review?(dkeeler)
Comment on attachment 8754669 [details]
MozReview Request: Bug 1222754 - Replace nsSecureBrowserUIImpl::mOnStateLocationChangeReentranceDetection and nsAutoAtomic.

https://reviewboard.mozilla.org/r/54160/#review51026

Neat - r=me.
Attachment #8754669 - Flags: review?(dkeeler) → review+
Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d0b230605dada321e90bb2f0678c5f0bbd71374
(AFAICT, all that orange is due to Bug 1270962, and other misc intermittent failures.)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2250f9d1fa6d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.