Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1867833003:
Prefer System Flash over non-local component updated Flash. (Closed)
|
Created:
4 years, 8 months ago by Will Harris Modified:
4 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, cpu_(ooo_6.6-7.5) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrefer System Flash over non-local component updated Flash.
This avoids attempting to load a Flash DLL off a network drive if doing
so would not result in loading an old version of Flash.
When determining which Flash to load the following algorithm is evaluated:
First, a candidate list of Flash versions is compiled sourced from:
- System Flash (Adobe updated) - (could be the debug version)
- Bundled Flash (ships with Chrome)
- Component updated Flash (updated on user data dir by component updater)*
* Note this really only applies on Linux when the component updated Flash
is already available at Chrome startup.
The highest single version will always win. If so, just use that version.
If there are two or more available version of Flash with the same version
then preference is given in the following order
- Flash content Debugger
- Bundled Flash
- Local-drive Component updated Flash
- System Flash
- Network-mounted component updated Flash
Because of the way that the pepperflash component updater injects its
version of Flash later on in Chrome load, the network drive determination
is done in two places, firstly inside ChromeContentClient when building
a candidate list, since Linux could have a component updated flash
ready for use at this point, and secondly when the pepperflash component
comes to inject the new version it will evalulate the order based on
the rule above on whether to inject itself.
TEST=https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul4s/preview
TEST=unit_tests --gtest_filter=ChromeContentClientTest.*
BUG=572131
Committed: https://crrev.com/73d6faae235add2deebf2a634f0cd0939751a946
Cr-Commit-Position: refs/heads/master@{#389006}
Patch Set 1 #Patch Set 2 : revamp #Patch Set 3 : rebase #
Total comments: 13
Patch Set 4 : rebase. split. upstream. #Patch Set 5 : code review comments #
Total comments: 6
Patch Set 6 : code review changes. also correct the display in plugins list #Patch Set 7 : fix test 9 #Patch Set 8 : fix mac #
Messages
Total messages: 46 (19 generated)
Description was changed from ========== Prefer System Flash over bundled or component updated Flash. If a system-level version of Flash is installed and is equal or greater to the versions of the Chrome bundled and component updated Flash, then prefer the system-level Flash. This avoids attempting to load component updated Flash off a network drive if the system-level Flash is also updated. BUG=512035,572131 ========== to ========== Prefer System Flash over bundled or component updated Flash. If a system-level version of Flash is installed and is equal or greater to the versions of the Chrome bundled and component updated Flash, then prefer the system-level Flash. This avoids attempting to load component updated Flash off a network drive if the system-level Flash is also updated. Since Flash debugger is always installed as system-level this subsumes the existing debugger override. BUG=512035,572131 ==========
wfh@chromium.org changed reviewers: + cpu@chromium.org
PTAL
actually don't take a look yet. I have decided to change the logic to make it less aggressive on picking the system flash.
Description was changed from ========== Prefer System Flash over bundled or component updated Flash. If a system-level version of Flash is installed and is equal or greater to the versions of the Chrome bundled and component updated Flash, then prefer the system-level Flash. This avoids attempting to load component updated Flash off a network drive if the system-level Flash is also updated. Since Flash debugger is always installed as system-level this subsumes the existing debugger override. BUG=512035,572131 ========== to ========== Prefer System Flash over non-local component updated Flash. This avoids loading a Flash DLL off a network drive if it does not reduce the user's security. When determining which Flash to load the following algorithm is evaluated First, a candidate list of Flash versions is compiled sourced from - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater) The highest single version will always win. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash during chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version. BUG=512035,572131 ==========
Description was changed from ========== Prefer System Flash over non-local component updated Flash. This avoids loading a Flash DLL off a network drive if it does not reduce the user's security. When determining which Flash to load the following algorithm is evaluated First, a candidate list of Flash versions is compiled sourced from - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater) The highest single version will always win. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash during chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version. BUG=512035,572131 ========== to ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=Lots, I'm going to write a testing plan. Testing this is hard. BUG=512035,572131 ==========
This now works, but I need to do a lot of testing. If you don't like the approach then say so now before I do a load of testing.
Description was changed from ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=Lots, I'm going to write a testing plan. Testing this is hard. BUG=512035,572131 ========== to ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=Lots, I'm going to write a testing plan. Testing this is hard. BUG=572131 ==========
Description was changed from ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=Lots, I'm going to write a testing plan. Testing this is hard. BUG=572131 ========== to ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul4s/preview BUG=572131 ==========
I added a testing plan - https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul...
hello cpu, I wondered if you had any comments on this CL, or could suggest someone else to review?
wfh@chromium.org changed reviewers: + thestig@chromium.org - cpu@chromium.org
+thestig I wonder if you could look at this, as you looked at some recent changes by kerrnel in the same area. Thanks!
cpu@chromium.org changed reviewers: + cpu@chromium.org
as usual, lets split the base changes in another CL. https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_un... base/files/file_util_unittest.cc:1679: /* hmmm ... rather use an environment variable, so even QA can test, in other words it it could be CHROME_TEST_FILE_SERVER or something like that. https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:578: return true; seems reasonable... but what if the file does not exist?
will split off the base changes and leave this CL for the Flash changes. https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:578: return true; On 2016/04/15 20:55:33, cpu wrote: > seems reasonable... but what if the file does not exist? files that don't exist are not on a network drive... :)
https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:566: FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, Do you need write and delete permissions? https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:575: if (::GetFileInformationByHandleEx(handle.Get(), Just return !!::GetFoo(...) ? https://codereview.chromium.org/1867833003/diff/40001/chrome/browser/componen... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1867833003/diff/40001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:162: PathService::Get(chrome::DIR_PEPPER_FLASH_PLUGIN, &bundled_flash_path); This can potentially fail. Do you care to check or can you just work with an empty |bundled_flash_path| ? https://codereview.chromium.org/1867833003/diff/40001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:166: for (auto plugin : plugins) { const auto & ? https://codereview.chromium.org/1867833003/diff/40001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1867833003/diff/40001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:475: if (!plugins.size()) if (plugins.empty())
This CL has now been split into two. the other one is https://codereview.chromium.org/1892153003 Will continue on this CL for Flash changes and other one for base changes. https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:566: FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, On 2016/04/15 21:16:36, Lei Zhang wrote: > Do you need write and delete permissions? These aren't file reading permissions these are file sharing permissions, it's a Windows-ism which basically is to do with other processes that might have the file open and yet still being able to read it. I should probably add a GENERIC_READ in there though. true. https://codereview.chromium.org/1867833003/diff/40001/base/files/file_util_wi... base/files/file_util_win.cc:575: if (::GetFileInformationByHandleEx(handle.Get(), On 2016/04/15 21:16:36, Lei Zhang wrote: > Just return !!::GetFoo(...) ? Acknowledged.
https://codereview.chromium.org/1867833003/diff/40001/chrome/browser/componen... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1867833003/diff/40001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:162: PathService::Get(chrome::DIR_PEPPER_FLASH_PLUGIN, &bundled_flash_path); On 2016/04/15 21:16:36, Lei Zhang wrote: > This can potentially fail. Do you care to check or can you just work with an > empty |bundled_flash_path| ? checking below, although FilePath::IsParent always returns false if the FilePath is empty. https://codereview.chromium.org/1867833003/diff/40001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:166: for (auto plugin : plugins) { On 2016/04/15 21:16:36, Lei Zhang wrote: > const auto & ? Done. https://codereview.chromium.org/1867833003/diff/40001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1867833003/diff/40001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:475: if (!plugins.size()) On 2016/04/15 21:16:36, Lei Zhang wrote: > if (plugins.empty()) Done.
lgtm https://codereview.chromium.org/1867833003/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1867833003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:172: !bundled_flash_path.empty() && bundled_flash_path.IsParent(plugin.path); 171-172 could go after 179, also 170 is duplicated with 175. https://codereview.chromium.org/1867833003/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1867833003/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:488: return plugin_map.rbegin()->second; mmm... clever but obscure, I don't know what the less() operator is implemented for tuples.. hopefully it does the right thing. At least lets keep the DCHECK. also shouldn't be using std::tuple?
cpu@chromium.org changed reviewers: + waffles@chromium.org
actually waffles should take a peek here.
lgtm If the sandbox won't load a plugin on a networked drive, why do we bother trying to register it (even if the version is higher)? https://codereview.chromium.org/1867833003/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1867833003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:159: bool is_on_network = base::IsOnNetworkDrive(path); nit: I would move this (and :171-172) down to just before they are used, or even just inline this.
On 2016/04/20 20:49:03, waffles wrote: > lgtm > > If the sandbox won't load a plugin on a networked drive, why do we bother trying > to register it (even if the version is higher)? because we want to "fail secure", e.g. if a Flash version has a vulnerability and we push a component update, we do not want users to load the insecure version of Flash even if their component path is on a network drive and updating it would break Flash. It's not an ideal situation unfortunately. This is covered by Test 3 in the test plan.
On 2016/04/20 20:54:05, Will Harris wrote: > On 2016/04/20 20:49:03, waffles wrote: > > lgtm > > > > If the sandbox won't load a plugin on a networked drive, why do we bother > trying > > to register it (even if the version is higher)? > > because we want to "fail secure", e.g. if a Flash version has a vulnerability > and we push a component update, we do not want users to load the insecure > version of Flash even if their component path is on a network drive and updating > it would break Flash. It's not an ideal situation unfortunately. This is covered > by Test 3 in the test plan. That's fine with me; I just wanted to point out that there's a little bit of overlap here: I thought that the way we prevent severely-compromised Flash from loading is through some kind of out-of-band plugin blacklisting, which is done in addition to the component update in cases of severe and urgent vulnerabilities. So, we already have a method to forbid old Flash - do we want a component update of Flash to imply that old Flash is forbidden? This will only ever matter if we're in a position where the preference order is New Flash > Old Flash > Broken Flash. Given the history, I think New Flash > Broken Flash > Old Flash is the more common preference order, but just want to point out that going this way locks us into the latter for networked-drive user-data-dirs.
On 2016/04/20 21:17:36, waffles wrote: > On 2016/04/20 20:54:05, Will Harris wrote: > > On 2016/04/20 20:49:03, waffles wrote: > > > lgtm > > > > > > If the sandbox won't load a plugin on a networked drive, why do we bother > > trying > > > to register it (even if the version is higher)? > > > > because we want to "fail secure", e.g. if a Flash version has a vulnerability > > and we push a component update, we do not want users to load the insecure > > version of Flash even if their component path is on a network drive and > updating > > it would break Flash. It's not an ideal situation unfortunately. This is > covered > > by Test 3 in the test plan. > > That's fine with me; I just wanted to point out that there's a little bit of > overlap here: I thought that the way we prevent severely-compromised Flash from > loading is through some kind of out-of-band plugin blacklisting, which is done > in addition to the component update in cases of severe and urgent > vulnerabilities. So, we already have a method to forbid old Flash - do we want a > component update of Flash to imply that old Flash is forbidden? This will only > ever matter if we're in a position where the preference order is New Flash > Old > Flash > Broken Flash. Given the history, I think New Flash > Broken Flash > Old > Flash is the more common preference order, but just want to point out that going > this way locks us into the latter for networked-drive user-data-dirs. waffles@ and I discussed this offline. I think the concerns are mitigated because previously users stuck in this situation had no way of fixing themselves. Now it's possible to just install the latest PPAPI system Flash and this will be preferred over the network component update, so hopefully users stuck in the New Flash > Broken Flash > Old Flash situation can just make New Flash be System Flash.
Description was changed from ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul4s/preview BUG=572131 ========== to ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul4s/preview TEST=unit_tests --gtest_filter=ChromeContentClientTest.* BUG=572131 ==========
Changes made. I also noticed that the display in chrome://plugins would now be wrong, for the system plugin case, so I made it display "System" or "Debug" appropriately. I updated the test plan. I'm going to do a full testing run now, in the meantime if you have any comments on the new files those would be appreciated. https://codereview.chromium.org/1867833003/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1867833003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:159: bool is_on_network = base::IsOnNetworkDrive(path); On 2016/04/20 20:49:03, waffles wrote: > nit: I would move this (and :171-172) down to just before they are used, or even > just inline this. Done. https://codereview.chromium.org/1867833003/diff/80001/chrome/browser/componen... chrome/browser/component_updater/pepper_flash_component_installer.cc:172: !bundled_flash_path.empty() && bundled_flash_path.IsParent(plugin.path); On 2016/04/20 19:23:14, cpu wrote: > 171-172 could go after 179, also 170 is duplicated with 175. Done. https://codereview.chromium.org/1867833003/diff/80001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1867833003/diff/80001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:488: return plugin_map.rbegin()->second; On 2016/04/20 19:23:14, cpu wrote: > mmm... clever but obscure, I don't know what the less() operator is implemented > for tuples.. hopefully it does the right thing. > > At least lets keep the DCHECK. also shouldn't be using std::tuple? yup less is implemented by "Compares lhs and rhs lexicographically, that is, compares the first elements, if they are equivalent, compares the second elements, if those are equivalent, compares the third elements, and so on." so this code should work. I was doubtful enough about it that I wrote a lot of tests. I added the DCHECK back again. Also, when this CL was made I don't think std:tuple was allowed, but now it is, so I changed it.
Update: I found an issue in testing whereby a component update of the same version as an installed copy of Debug Flash will always takes precedence over the Debug Flash, and this appears to be a regression so I'll fix that and upload a new PS.
Patchset #7 (id:120001) has been deleted
bug is now fixed in ps6 and I went through a complete testing run. I think this is now safe to commit. Let me know if you have any additional comments.
The CQ bit was checked by wfh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, cpu@chromium.org Link to the patchset: https://codereview.chromium.org/1867833003/#ps160001 (title: "fix mac")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867833003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867833003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867833003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867833003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867833003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867833003/160001
Message was sent while issue was closed.
Description was changed from ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul4s/preview TEST=unit_tests --gtest_filter=ChromeContentClientTest.* BUG=572131 ========== to ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul4s/preview TEST=unit_tests --gtest_filter=ChromeContentClientTest.* BUG=572131 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul4s/preview TEST=unit_tests --gtest_filter=ChromeContentClientTest.* BUG=572131 ========== to ========== Prefer System Flash over non-local component updated Flash. This avoids attempting to load a Flash DLL off a network drive if doing so would not result in loading an old version of Flash. When determining which Flash to load the following algorithm is evaluated: First, a candidate list of Flash versions is compiled sourced from: - System Flash (Adobe updated) - (could be the debug version) - Bundled Flash (ships with Chrome) - Component updated Flash (updated on user data dir by component updater)* * Note this really only applies on Linux when the component updated Flash is already available at Chrome startup. The highest single version will always win. If so, just use that version. If there are two or more available version of Flash with the same version then preference is given in the following order - Flash content Debugger - Bundled Flash - Local-drive Component updated Flash - System Flash - Network-mounted component updated Flash Because of the way that the pepperflash component updater injects its version of Flash later on in Chrome load, the network drive determination is done in two places, firstly inside ChromeContentClient when building a candidate list, since Linux could have a component updated flash ready for use at this point, and secondly when the pepperflash component comes to inject the new version it will evalulate the order based on the rule above on whether to inject itself. TEST=https://docs.google.com/document/d/1iTQiaqjuHsKV4cPqSOet-eJKWb2SsJLp2ieDj_Mul4s/preview TEST=unit_tests --gtest_filter=ChromeContentClientTest.* BUG=572131 Committed: https://crrev.com/73d6faae235add2deebf2a634f0cd0939751a946 Cr-Commit-Position: refs/heads/master@{#389006} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/73d6faae235add2deebf2a634f0cd0939751a946 Cr-Commit-Position: refs/heads/master@{#389006} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
