Closed
Bug 1433265
Opened 6 years ago
Closed 6 years ago
nsLocalFileWin.cpp:ShortcutResolver needs to go
Categories
(Core :: XPCOM, enhancement, P3)
Core
XPCOM
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.
Reporter | ||
Updated•6 years ago
|
status-firefox58:
affected → ---
status-firefox59:
affected → ---
status-firefox60:
affected → ---
Priority: -- → P3
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
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
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d7c27819bb20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•