Closed Bug 1110935 Opened 5 years ago Closed 4 years ago

Remove locking from nsSecureBrowserUIImpl

Categories

(Core Graveyard :: Security: UI, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: evilpie, Assigned: Cykesiopka)

References

Details

Attachments

(3 files, 3 obsolete files)

Apparently nsSecureBrowserUIImpl is just used from the mainthread nowadays. Removing all the instances of ReentrantMonitorAutoEnter and temp_ variables should make this code a lot more pleasant.
Bug 1110935 - Part 1: Assert we're on the main thread on public methods.
Attachment #8681970 - Flags: review?(dkeeler)
Bug 1110935 - Part 2: Remove ReentrantMonitor and ReentrantMonitorAutoEnter uses.
Attachment #8681971 - Flags: review?(dkeeler)
Bug 1110935 - Part 3: Remove now unnecessary temp variables.
Attachment #8681972 - Flags: review?(dkeeler)
Things look OK so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23f8298bba7d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6c73da324fc
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Comment on attachment 8681970 [details]
MozReview Request: Bug 1110935 - Part 1: Assert we're on the main thread on public methods.

https://reviewboard.mozilla.org/r/23895/#review21405

Looks good. I think we can also replace the NS_DECL_THREADSAFE_ISUPPORTS with NS_DECL_ISUPPORTS in nsSecureBrowserUIImpl.h
Attachment #8681970 - Flags: review?(dkeeler) → review+
Comment on attachment 8681971 [details]
MozReview Request: Bug 1110935 - Part 2: Remove ReentrantMonitor and ReentrantMonitorAutoEnter uses.

https://reviewboard.mozilla.org/r/23897/#review21407

::: security/manager/ssl/nsSecureBrowserUIImpl.h:42
(Diff revision 1)
> -  
> +

tiny minor nit: not sure we need a blank line here

::: 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.
Attachment #8681971 - Flags: review?(dkeeler) → review+
Comment on attachment 8681972 [details]
MozReview Request: Bug 1110935 - Part 3: Remove now unnecessary temp variables.

https://reviewboard.mozilla.org/r/23899/#review21415

Great!
https://reviewboard.mozilla.org/r/23897/#review21407

> 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.

Yeah, I wasn't sure about whether it was ok to remove, so I left it alone. I'll file a follow-up later.
+ s/NS_DECL_THREADSAFE_ISUPPORTS/NS_DECL_ISUPPORTS/ in nsSecureBrowserUIImpl.h
Attachment #8681970 - Attachment is obsolete: true
Attachment #8682330 - Flags: review+
+ Address whitespace nit
Attachment #8681971 - Attachment is obsolete: true
Attachment #8682331 - Flags: review+
Attachment #8681972 - Attachment is obsolete: true
Attachment #8682332 - Flags: review+
Thanks for the review!

(Try push is in comment 4).
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/38c38564a12e
https://hg.mozilla.org/mozilla-central/rev/7fc3ebd52cbf
https://hg.mozilla.org/mozilla-central/rev/8ef27a178bc2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.