Closed Bug 1433265 Opened 6 years ago Closed 6 years ago

nsLocalFileWin.cpp:ShortcutResolver needs to go

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: froydnj, Assigned: emk)

References

Details

Attachments

(1 file)

...if it can:

http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#292

We have a single global instance of this class for what appears to be the sole purpose of resolving shortcuts.  I am not an expert at Windows filesystem operations, but surely resolving shortcuts can't be *this* difficult?

aklotz pointed out that it might have been difficult in an XP world, but now that we only support Win7+, we should look into seeing whether we can clean this code up.
Unhiding by request from froydnj.
Group: core-security
Priority: -- → P3
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Blocks: 1428557
Comment on attachment 8947081 [details]
Bug 1433265 - Remove ShortcutResolver from nsLocalFileWin.cpp.

https://reviewboard.mozilla.org/r/216878/#review222712

Thank you for doing this!  I have a few questions; assuming that they all have reasonable answers, r=me.

::: xpcom/io/nsILocalFileWin.idl
(Diff revision 1)
> -     * setShortcut
> -     *
> -     * Creates the specified shortcut, or updates it if it already exists.

I assume that we can safely remove this because nothing in C++ or chrome code calls this, and we don't have to worry about addons, yes?

::: xpcom/io/nsLocalFileWin.cpp
(Diff revision 1)
> -};
> -
> -ShortcutResolver::ShortcutResolver() :
> -  mLock("ShortcutResolver.mLock")
> -{
> -  CoInitialize(nullptr);

How certain are we that somebody else takes care of this for us?  I see the same calls in `nsAppRunner.cpp`, somewhere in `gfx/`, and a couple of other places, so I guess we don't have to worry about this?

::: xpcom/io/nsLocalFileWin.cpp
(Diff revision 1)
> -{
> -  if (!mShellLink) {
> -    return NS_ERROR_FAILURE;
> -  }
> -
> -  MutexAutoLock lock(mLock);

To be clear, we don't need the lock anymore, because all the state we're keeping for each `Resolve` call is local to the thread, correct?

::: xpcom/tests/unit/xpcshell.ini
(Diff revision 1)
> -[test_windows_shortcut.js]
> -skip-if = os != "win"

Why are we removing this test?
Attachment #8947081 - Flags: review?(nfroyd) → review+
Comment on attachment 8947081 [details]
Bug 1433265 - Remove ShortcutResolver from nsLocalFileWin.cpp.

https://reviewboard.mozilla.org/r/216878/#review222712

> I assume that we can safely remove this because nothing in C++ or chrome code calls this, and we don't have to worry about addons, yes?

Yes. setShortcut was added for webapprt that is now dead.

> How certain are we that somebody else takes care of this for us?  I see the same calls in `nsAppRunner.cpp`, somewhere in `gfx/`, and a couple of other places, so I guess we don't have to worry about this?

XRE_main has a MainThreadRuntime object at the very begining of the function.[1] MainThreadRuntime has a STARegion member.[2] STARegion will automatically call CoInit/CoUninit (it's the sole puopose of this class.[3]) So basically don't have to worry about this.
[1] https://dxr.mozilla.org/mozilla-central/rev/5454ed95c82a956009db2f4b04d008ec8753e61e/toolkit/xre/nsAppRunner.cpp#4817
[2] https://dxr.mozilla.org/mozilla-central/rev/5454ed95c82a956009db2f4b04d008ec8753e61e/ipc/mscom/MainThreadRuntime.h#50
[3] https://dxr.mozilla.org/mozilla-central/rev/5454ed95c82a956009db2f4b04d008ec8753e61e/ipc/mscom/COMApartmentRegion.h#23,32,55

> To be clear, we don't need the lock anymore, because all the state we're keeping for each `Resolve` call is local to the thread, correct?

Correct. By the way, the current code is violating the COM threading model. Since gResolver belongs to the main STA, non-main-thread callers have to martial the interface pointer to use it. Currently it happens to work by a lucky accident. This patch will also fix the violation.

> Why are we removing this test?

Because it entirely tests setShortcut that this patch removed.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/d7c27819bb20
Remove ShortcutResolver from nsLocalFileWin.cpp. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/d7c27819bb20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.