Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
117123002:
Fixing Firefox 21+ password import (Closed)
|
Created:
7 years ago by mpawlowski Modified:
6 years, 11 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFixing Firefox 21+ password import
Firefox 21 changed the directory layout within Firefox.app.
Some libs and content were moved from Firefox.app/Content/MacOS to
Firefox.app/Content/MacOS/browser, and this new location was
read as app_path_ by the importer. However, libnss3.dylib is still
in Firefox.app/Content/MacOS.
Changing utils to set app_path_ based on compatibility.ini's
LastPlatformDir (which always has Firefox.app/Content/MacOS)
instead of LastAppDir (which was changed to
Firefox.app/Content/MacOS/browser).
'searchplugins' subdir was moved into the new 'browser' subdir
so search engine importing code has to be adjusted.
Enabling Firefox importer browser tests on Mac. They didn't
work because they attempted to use Windows nss libraries.
They should have been using Mac-specific libraries which are
already available for Mac unit tests.
BUG=321112, 48007
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245198
Patch Set 1 #
Total comments: 7
Patch Set 2 : Tweaks in GetSearchEnginesXMLData #
Total comments: 5
Patch Set 3 : Curly braces around multi-line if #Patch Set 4 : Build path up explicitly from .app #
Total comments: 5
Patch Set 5 : Build path using base::FilePath functionality #Patch Set 6 : Compilation fix #Patch Set 7 : Revert enabling browser tests on Mac #
Messages
Total messages: 25 (0 generated)
Less changes needed than in the previous attempt :)
Thanks for digging into this! https://codereview.chromium.org/117123002/diff/1/chrome/browser/importer/fire... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/117123002/diff/1/chrome/browser/importer/fire... chrome/browser/importer/firefox_importer_browsertest.cc:239: data_path = data_path.AppendASCII("firefox3_nss_mac"); I think this needs to be part of the CL as well? https://codereview.chromium.org/117123002/diff/1/chrome/common/importer/firef... File chrome/common/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/117123002/diff/1/chrome/common/importer/firef... chrome/common/importer/firefox_importer_utils.cc:118: } else if (key == "LastPlatformDir") { Do you know what the actual semantic meaning of this key is? My concern is just that we're using this value because it happens to contain the directory we need and not because of it's real meaning? My suggestion would be for the caller to look at the value of LastAppDir, search for Firefox.app manually and build up the path from there rather than switching to a key which happens to contain the directory we're looking for. https://codereview.chromium.org/117123002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/117123002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:408: base::FilePath app_path = app_path_.AppendASCII("browser/searchplugins"); I'm uncomfortable with the static initialization using mutliple path components, do we do this elsewhere in the code? if so np. Otherwise, consdier using .AppendASCII() to add path components. https://codereview.chromium.org/117123002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:408: base::FilePath app_path = app_path_.AppendASCII("browser/searchplugins"); Consider splitting into a new searchplugins_path and then building app_path out of it?
https://codereview.chromium.org/117123002/diff/1/chrome/browser/importer/fire... File chrome/browser/importer/firefox_importer_browsertest.cc (right): https://codereview.chromium.org/117123002/diff/1/chrome/browser/importer/fire... chrome/browser/importer/firefox_importer_browsertest.cc:239: data_path = data_path.AppendASCII("firefox3_nss_mac"); On 2013/12/17 11:48:09, jeremy wrote: > I think this needs to be part of the CL as well? Added a sentence in the CL. https://codereview.chromium.org/117123002/diff/1/chrome/common/importer/firef... File chrome/common/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/117123002/diff/1/chrome/common/importer/firef... chrome/common/importer/firefox_importer_utils.cc:118: } else if (key == "LastPlatformDir") { On 2013/12/17 11:48:09, jeremy wrote: > Do you know what the actual semantic meaning of this key is? > > My concern is just that we're using this value because it happens to contain the > directory we need and not because of it's real meaning? > > My suggestion would be for the caller to look at the value of LastAppDir, search > for Firefox.app manually and build up the path from there rather than switching > to a key which happens to contain the directory we're looking for. What if someone renames Firefox.app? From what I can tell by looking at Firefox's code, LastPlatformDir is the folder in which the XUL runtime is stored and LastAppDir is the folder with omni.ja - the package with resources, UI definitions etc., the actual "app". The question which one should be used to find what we need for importing is open but IMHO we don't have any control over this and Mozilla seems to move stuff about. We should use whatever works and worry again when it breaks, we won't be able to foresee what kind of rearrangement they'll come up with next, no assumption is guaranteed to be correct. I could change this to any arbitrary method of arriving at Firefox.app/Content/MacOS but it won't guarantee they don't move those libs to Firefox.app/Content/MacOS/Libraries next week. https://codereview.chromium.org/117123002/diff/1/chrome/utility/importer/fire... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/117123002/diff/1/chrome/utility/importer/fire... chrome/utility/importer/firefox_importer.cc:408: base::FilePath app_path = app_path_.AppendASCII("browser/searchplugins"); On 2013/12/17 11:48:09, jeremy wrote: > I'm uncomfortable with the static initialization using mutliple path components, > do we do this elsewhere in the code? if so np. > > Otherwise, consdier using .AppendASCII() to add path components. We seem to be using it all over the place in various tests, didn't find usages in production code. I'll change it.
https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... File chrome/common/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:118: } else if (key == "LastPlatformDir") { Two questions: (1) Does this work for all versions of Firefox, or just newer ones? (2) Does this work for all platforms, or just Mac? https://codereview.chromium.org/117123002/diff/20001/chrome/utility/importer/... File chrome/utility/importer/firefox_importer.cc (right): https://codereview.chromium.org/117123002/diff/20001/chrome/utility/importer/... chrome/utility/importer/firefox_importer.cc:418: app_path = app_path_.Append(searchplugins_path); nit: Please add curly braces, since the body of this if-stmt spans multiple lines of text.
https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... File chrome/common/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:118: } else if (key == "LastPlatformDir") { On 2013/12/17 23:04:14, Ilya Sherman wrote: > Two questions: > > (1) Does this work for all versions of Firefox, or just newer ones? > (2) Does this work for all platforms, or just Mac? I've looked at Firefox's sources, and the code that sets those variables hasn't been touched since August 2005 (https://github.com/mozilla/gecko-dev/blame/master/toolkit/xre/nsAppRunner.cpp search page for "LastPlatformDir") which is about when Firefox 1.0.6 was released :) This is the only place in their codebase where "LastPlatformDir" and "LastAppDir" are used, so it's safe to assume this logic is cross-platform. I've checked several Firefox versions I had on Mac and Windows, all have this. As for the effect of this change on Chrome platforms, everywhere I checked (Windows, Ubuntu, Mac) the actual FF libraries are under LastPlatformDir and LastAppDir points to LastPlatformDir/browser, so it's the same as observed on Mac. This should be a beneficial change everywhere. However, on Windows we use the registry to find the installation path, not this ini file, and on Ubuntu we seem to be using the system NSS decryptor and we ignore Firefox's installation altogether. So my fix, while valid everywhere, will go unnoticed on those platforms - they haven't been affected by Mozilla's changes and won't be affected by my fix.
Thanks! LGTM for my drive-by. Please also wait for Jeremy's LG in case he has any additional comments. https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... File chrome/common/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:118: } else if (key == "LastPlatformDir") { On 2013/12/18 08:50:29, mpawlowski wrote: > On 2013/12/17 23:04:14, Ilya Sherman wrote: > > Two questions: > > > > (1) Does this work for all versions of Firefox, or just newer ones? > > (2) Does this work for all platforms, or just Mac? > > I've looked at Firefox's sources, and the code that sets those variables hasn't > been touched since August 2005 > (https://github.com/mozilla/gecko-dev/blame/master/toolkit/xre/nsAppRunner.cpp > search page for "LastPlatformDir") which is about when Firefox 1.0.6 was > released :) This is the only place in their codebase where "LastPlatformDir" and > "LastAppDir" are used, so it's safe to assume this logic is cross-platform. I've > checked several Firefox versions I had on Mac and Windows, all have this. > > As for the effect of this change on Chrome platforms, everywhere I checked > (Windows, Ubuntu, Mac) the actual FF libraries are under LastPlatformDir and > LastAppDir points to LastPlatformDir/browser, so it's the same as observed on > Mac. This should be a beneficial change everywhere. However, on Windows we use > the registry to find the installation path, not this ini file, and on Ubuntu we > seem to be using the system NSS decryptor and we ignore Firefox's installation > altogether. So my fix, while valid everywhere, will go unnoticed on those > platforms - they haven't been affected by Mozilla's changes and won't be > affected by my fix. Lovely. Thanks for doing the deep investigation there!
https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... File chrome/common/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:118: } else if (key == "LastPlatformDir") { I understand that this matches what we want and hasn't changed in a very long time, but what is the semantic meaning of LastPlatformDir? IMHO if we aren't using this for the intended purpose, it makes more sense to find the .app ourselves in the path and build up the path to the library from there. I just feel that it's more robust than relying on things not changing.
On 2013/12/18 20:16:08, jeremy wrote: > https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... > File chrome/common/importer/firefox_importer_utils.cc (right): > > https://codereview.chromium.org/117123002/diff/20001/chrome/common/importer/f... > chrome/common/importer/firefox_importer_utils.cc:118: } else if (key == > "LastPlatformDir") { > I understand that this matches what we want and hasn't changed in a very long > time, but what is the semantic meaning of LastPlatformDir? > > IMHO if we aren't using this for the intended purpose, it makes more sense to > find the .app ourselves in the path and build up the path to the library from > there. I just feel that it's more robust than relying on things not changing. As I've said, LastPlatformDir is (from what I can tell by looking at Mozilla's code) the path to where the XUL runtime sits, LastAppDir is where the app definitions sit. XUL is a framework that draws UI based on content. The definitions of how a Mozilla app looks and behaves are stored mostly in $LastAppDir/omni.ja archive (as opposed to being mostly hardcoded in C++). So LastAppDir points to the app's UI content, LastPlatformDir points to the runtime interpreter of this content. Neither points explicitly to where various libs will sit, so using either is equally arbitrary. It's also equally arbitrary to try and build the path up from the .app. For all we know, Mozilla is as likely to change the meaning of LastPlatformDir as it is to move XUL and libs into .app/Content/MacOS/binaries or wherever. My approach assumes libs will always be in the same dir as XUL, yours that they will always be in .app/Content/MacOS, both are just guesses. I submit that it's impossible to predict what they'll change next and it's futile to try to future-proof this code too much.
On 2013/12/19 08:27:03, mpawlowski wrote: > As I've said, LastPlatformDir is (from what I can tell by looking at Mozilla's > code) the path to where the XUL runtime sits, LastAppDir is where the app > definitions sit. XUL is a framework that draws UI based on content. The > definitions of how a Mozilla app looks and behaves are stored mostly in > $LastAppDir/omni.ja archive (as opposed to being mostly hardcoded in C++). So > LastAppDir points to the app's UI content, LastPlatformDir points to the runtime > interpreter of this content. Neither points explicitly to where various libs > will sit, so using either is equally arbitrary. > > It's also equally arbitrary to try and build the path up from the .app. For all > we know, Mozilla is as likely to change the meaning of LastPlatformDir as it is > to move XUL and libs into .app/Content/MacOS/binaries or wherever. My approach > assumes libs will always be in the same dir as XUL, yours that they will always > be in .app/Content/MacOS, both are just guesses. I submit that it's impossible > to predict what they'll change next and it's futile to try to future-proof this > code too much. I agree wholeheartedly with the last sentence here, it is indeed impossible and futile to completely future-proof this code. My preference comes down to being explicit about the path we're using rather than implicitly depending on the value of this key which semantically makes no guarantees about it's relation to the location of libnss. I'm grateful for your efforts to dig down to the bottom of this and fix this issue and I really don't want to create more work for you but this comes down to a judgement call. As the reviewer my judgement is to build up the path explicitly from the location of the .app, this should be a pretty simple change. This is a dark corner of the code, I really want to make this as robust and explicit as possible - let me again apologise for being opinionated on this but I do feel this is important.
Just a heads-up, the requested changes have been submitted and await reviewing.
CL looks like it's missing FF dlls? https://codereview.chromium.org/117123002/diff/60001/chrome/common/importer/f... File chrome/common/importer/firefox_importer_utils.cc (right): https://codereview.chromium.org/117123002/diff/60001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:109: std::string& line = lines[i]; maintain const and work on a copy of the string below in the #ifdef https://codereview.chromium.org/117123002/diff/60001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:118: } else if (key == "LastPlatformDir") { Why are we changing this? This code is cross-platform, are we sure changing the key doesn't break other platforms? LastAppDir contains the .app path, right? https://codereview.chromium.org/117123002/diff/60001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:123: #if defined (OS_MACOSX) s/defined (/defined(/ https://codereview.chromium.org/117123002/diff/60001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:130: const size_t pos = line.find(dot_app); Recommend manipulating as a FilePath: * Split using GetComponents(). * then build up a new FilePath using Append() to iteratively add path components. https://codereview.chromium.org/117123002/diff/60001/chrome/common/importer/f... chrome/common/importer/firefox_importer_utils.cc:140: #endif #endif // OS_MACOSX
On 2014/01/07 12:37:27, jeremy wrote: > CL looks like it's missing FF dlls? They were already in the repo, they just weren't referenced correctly by the tests.
bump?
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpawlowski@opera.com/117123002/130001
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
On 2014/01/14 10:44:55, I haz the power (commit-bot) wrote: > Retried try job too often on win_rel for step(s) base_unittests, browser_tests, > interactive_ui_tests, net_unittests, unit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Uploaded compilation fix
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpawlowski@opera.com/117123002/390001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
I've looked at why the Mac browser tests fail and apparently they were disabled for a different reason than I initially thought. In the test, we copy the .dylib files from DIR_TEST_DATA to a temporary directory in which the NSSDecryptor is supposed to find them. There's some weirdness there (ex. the files are copied to the "profile" subdirectory , not the "app" subdirectory - this might very well be the only reason why firefox_importer checks the profile folder for libraries) but that's not the biggest issue. The biggest issue is that libnss3.dylib has a dependency on libnssutil3.dylib. The dynamic linker will search for this dependency in @executable_path/libnssutil3.dylib, where @executable_path is the path to the calling executable (browser_tests), not to the dependant library. In production code we deal with it by specifying a fallback location, the environmental variable of DYLD_FALLBACK_LIBRARY_PATH. This variable is set by external_process_importer_client.cc by a call to a non-mocked GetFirefoxDylibPath() function, which basically returns the path to the actual Firefox installation even in tests. This means that when the browser_tests exec tries loading libnssutil3.dylib to satisfy libnss3's dependencies, it first searches for it in src/out/Debug/ or something along these lines, then in /Users/you/Desktop/Firefox.app or wherever the real Firefox is installed, then it fails. It doesn't search in the temporary directory to which we've copied it. This test only passes if Firefox 3.5 happens to be installed on the machine that runs it. Changing the way this works seems very hard, see http://stackoverflow.com/questions/5399012/overriding-executable-path-in-a-dl... Firefox 3.0 tests work because they don't need a to decode passwords. This version used a very insecure method of storing passwords. There is a light in the tunnel though - newer Firefox has apparently consolidated the deps of libnss3.dylib into it, so it can be loaded by dlopen without any trickery with DYLD_FALLBACK_LIBRARY_PATH or anything like that. In fact, if I just replace libnss3.dylib in our DIR_TEST_DATA/firefox3_nss_mac with the one from a new Firefox package, the test passes. This is the simplest way to get it working. I can either turn these tests back off for Mac or replace them with Firefox 26 tests, I can't get Firefox 3.5 tests to work on Mac. What do you think?
IMHO land this patch as is with the tests disabled and then add the firefox 26 libs in a followup patch with the test running against them. On Wed, Jan 15, 2014 at 11:43 AM, <mpawlowski@opera.com> wrote: > I've looked at why the Mac browser tests fail and apparently they were > disabled > for a different reason than I initially thought. > > In the test, we copy the .dylib files from DIR_TEST_DATA to a temporary > directory in which the NSSDecryptor is supposed to find them. There's some > weirdness there (ex. the files are copied to the "profile" subdirectory , > not > the "app" subdirectory - this might very well be the only reason why > firefox_importer checks the profile folder for libraries) but that's not > the > biggest issue. The biggest issue is that libnss3.dylib has a dependency on > libnssutil3.dylib. The dynamic linker will search for this dependency in > @executable_path/libnssutil3.dylib, where @executable_path is the path to > the > calling executable (browser_tests), not to the dependant library. In > production > code we deal with it by specifying a fallback location, the environmental > variable of DYLD_FALLBACK_LIBRARY_PATH. This variable is set by > external_process_importer_client.cc by a call to a non-mocked > GetFirefoxDylibPath() function, which basically returns the path to the > actual > Firefox installation even in tests. This means that when the browser_tests > exec > tries loading libnssutil3.dylib to satisfy libnss3's dependencies, it first > searches for it in src/out/Debug/ or something along these lines, then in > /Users/you/Desktop/Firefox.app or wherever the real Firefox is installed, > then > it fails. It doesn't search in the temporary directory to which we've > copied it. > This test only passes if Firefox 3.5 happens to be installed on the > machine that > runs it. > > Changing the way this works seems very hard, see > http://stackoverflow.com/questions/5399012/overriding- > executable-path-in-a-dll-loaded-with-dlopen > Firefox 3.0 tests work because they don't need a to decode passwords. This > version used a very insecure method of storing passwords. > > There is a light in the tunnel though - newer Firefox has apparently > consolidated the deps of libnss3.dylib into it, so it can be loaded by > dlopen > without any trickery with DYLD_FALLBACK_LIBRARY_PATH or anything like > that. In > fact, if I just replace libnss3.dylib in our > DIR_TEST_DATA/firefox3_nss_mac with > the one from a new Firefox package, the test passes. This is the simplest > way to > get it working. > > I can either turn these tests back off for Mac or replace them with > Firefox 26 > tests, I can't get Firefox 3.5 tests to work on Mac. What do you think? > > https://codereview.chromium.org/117123002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpawlowski@opera.com/117123002/600001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpawlowski@opera.com/117123002/600001
Message was sent while issue was closed.
Change committed as 245198 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
