ResourceBundle on Linux, and package manager changing files from under the browser

[Paweł Hajdan, Jr.]

I'm working on https://code.google.com/p/chromium/issues/detail?id=295103 , which is related to Linux SxS (side-by-side) migration, but also affects current Chrome Linux packages but in a more subtle way.

The problem is that ResourceBundle lazily loads resources from disk when requested. The resources might have been removed from under the browser (as is the case with above bug), but it's also possible even without SxS that the package manager has done an upgrade and the files on disk are out of sync with running version of Chrome.

One easy way to deal with this problem is to say it's inevitable. It's indeed non-trivial to fix since even if we load all the resources at startup (which is also not that easy since the list of resources to load needs to be complete and stay accurate; also it'd likely regress startup performance) there'd still be a small window of time between chrome binary being loaded into memory and resource files being loaded from disk during which the resource files on disk may become out of sync with the running binary.

One solution to this problem would be to bake the resources into chrome binary. This might also regress startup performance, but generally it should result in lazy loading of resources and should guarantee things staying in sync.

What are your opinions, thoughts and recommendations about this? Please let me know if I should explain the problem better.

Pawel

[Scott Hess]

This is an odd bug to get so late in the game - I thought the entire point of bundling resources up and opening them so that the file descriptor can be shared by the zygote was to avoid this problem?  Has something changed?  I think your debug build could easily not reproduce production on this front.

-scott

[Antoine Labour]

On Tue, Sep 24, 2013 at 2:00 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Opening the files as soon as possible after startup (without impacting the critical path for startup) should be good enough. Yes, it's in theory racy if the update happens at exactly the time you run the browser, but in practice, it should be good enough.

In many places we go to lengths to open files early and keep them open. E.g. that's the point of the zygote, and the reason we fork&exec /proc/self/exe when we can't use it (e.g. gpu process).

Antoine

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Elliot Glaysher (Chromium)]

On Tue, Sep 24, 2013 at 2:09 PM, Scott Hess <sh...@chromium.org> wrote:

This is an odd bug to get so late in the game - I thought the entire point of bundling resources up and opening them so that the file descriptor can be shared by the zygote was to avoid this problem?  Has something changed?  I think your debug build could easily not reproduce production on this front.

This is my understanding of how things were supposed to work. We opened the resource bundle early on and held onto the file descriptor so that if the file changed out from under us, we still held a reference to the version that was there when we opened.

-- Elliot

[Mattias Nissler]

That sounds all nice and fine, but doesn't it assume that the process that clobbers the file actually does unlink/move in a new version instead of just opening and clobbering file contents? Maybe that's what Paweł is seeing? One way of preventing clobbering could be to make a copy of the file, open that, and unlink the copy - that'd only be fast on file systems that do copy-on-write behind the scenes though.

-- Elliot

[Antoine Labour]

On Wed, Sep 25, 2013 at 2:53 AM, Mattias Nissler <mnis...@chromium.org> wrote:

On Tue, Sep 24, 2013 at 11:28 PM, Elliot Glaysher (Chromium) <e...@chromium.org> wrote:

On Tue, Sep 24, 2013 at 2:09 PM, Scott Hess <sh...@chromium.org> wrote:

This is an odd bug to get so late in the game - I thought the entire point of bundling resources up and opening them so that the file descriptor can be shared by the zygote was to avoid this problem?  Has something changed?  I think your debug build could easily not reproduce production on this front.

This is my understanding of how things were supposed to work. We opened the resource bundle early on and held onto the file descriptor so that if the file changed out from under us, we still held a reference to the version that was there when we opened.

That sounds all nice and fine, but doesn't it assume that the process that clobbers the file actually does unlink/move in a new version instead of just opening and clobbering file contents? Maybe that's what Paweł is seeing? One way of preventing clobbering could be to make a copy of the file, open that, and unlink the copy - that'd only be fast on file systems that do copy-on-write behind the scenes though.

We're dependent on the package manager to do the right thing, but AIUI apt and yum do it.

Antoine

 

-- Elliot

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Scott Hess]

On Wed, Sep 25, 2013 at 10:00 AM, Antoine Labour <pi...@google.com> wrote:

On Wed, Sep 25, 2013 at 2:53 AM, Mattias Nissler <mnis...@chromium.org> wrote:

On Tue, Sep 24, 2013 at 11:28 PM, Elliot Glaysher (Chromium) <e...@chromium.org> wrote:

On Tue, Sep 24, 2013 at 2:09 PM, Scott Hess <sh...@chromium.org> wrote:

This is an odd bug to get so late in the game - I thought the entire point of bundling resources up and opening them so that the file descriptor can be shared by the zygote was to avoid this problem?  Has something changed?  I think your debug build could easily not reproduce production on this front.

This is my understanding of how things were supposed to work. We opened the resource bundle early on and held onto the file descriptor so that if the file changed out from under us, we still held a reference to the version that was there when we opened.

That sounds all nice and fine, but doesn't it assume that the process that clobbers the file actually does unlink/move in a new version instead of just opening and clobbering file contents? Maybe that's what Paweł is seeing? One way of preventing clobbering could be to make a copy of the file, open that, and unlink the copy - that'd only be fast on file systems that do copy-on-write behind the scenes though.

We're dependent on the package manager to do the right thing, but AIUI apt and yum do it.

In my experience, something which updated the files in place like that would be a real nightmare.  The unlink/replace strategy is generally used exactly because it allows any outstanding file handles to keep working correctly, more-or-less, kinda-sorta.  Even things like rsync use that strategy unless you specifically tell it to update in place.

We should certainly determine if this is what's actually happening before doing any work in that direction.  It would seem more likely that someone added a new resource somehow but didn't add the early open to pin things down.

-scott

[Paweł Hajdan, Jr.]

On Wed, Sep 25, 2013 at 10:40 AM, Scott Hess <sh...@chromium.org> wrote:

It would seem more likely that someone added a new resource somehow but didn't add the early open to pin things down.

I think that's the case and I'll investigate more in that direction.

Note that I'm pretty sure we're not truncating and re-filling the files in place here. For my quick repro I'm just deleting the files. If they've been loaded into memory / opened nothing should break. For what the bug is about exactly, the files are moved to a different location, not overwritten in any way.

Pawel

[Paweł Hajdan, Jr.]

More into my investigation it seems that each renderer loads the files freshly from disk (I've changed log in data_pack.cc to FATAL):

[10052:10052:0925/140559:ERROR:memory_mapped_file.cc(51)] Couldn't open /usr/local/google/home/phajdan/chromium/src/out/Debug/chrome_100_percent.pak

[10052:10052:0925/140559:FATAL:data_pack.cc(78)] Failed to mmap datapack

 [0x555556b32da0] base::debug::StackTrace::StackTrace()

 [0x555556b6dd2b] logging::LogMessage::~LogMessage()

 [0x5555572c54e6] ui::DataPack::LoadFromPath()

 [0x5555572c9790] ui::ResourceBundle::AddDataPackFromPathInternal()

 [0x5555572c776b] ui::ResourceBundle::AddDataPackFromPath()

 [0x5555572cda4c] ui::ResourceBundle::LoadCommonResources()

 [0x5555572c740d] ui::ResourceBundle::InitSharedInstanceWithLocale()

 [0x555555d6f4cf] ChromeMainDelegate::PreSandboxStartup()

 [0x555556b16d48] content::ContentMainRunnerImpl::Initialize()

 [0x555556b154fe] content::ContentMain()

 [0x555555d6e09d] ChromeMain

 [0x555555d6e05c] main

 [0x7ffff1e3476d] __libc_start_main

 [0x555555d6df69] <unknown>

In the chrome_main_delegate.cc I see the following code:

#if defined(OS_ANDROID)
    // The renderer sandbox prevents us from accessing our .pak files directly.
    // Therefore file descriptors to the .pak files that we need are passed in
    // at process creation time.
    int locale_pak_fd = base::GlobalDescriptors::GetInstance()->MaybeGet(
        kAndroidLocalePakDescriptor);
    CHECK(locale_pak_fd != -1);
    ResourceBundle::InitSharedInstanceWithPakFile(locale_pak_fd, false);

    int extra_pak_keys[] = {
      kAndroidChrome100PercentPakDescriptor,
      kAndroidUIResourcesPakDescriptor,
    };
    for (size_t i = 0; i < arraysize(extra_pak_keys); ++i) {
      int pak_fd =
          base::GlobalDescriptors::GetInstance()->MaybeGet(extra_pak_keys[i]);
      CHECK(pak_fd != -1);
      ResourceBundle::GetSharedInstance().AddDataPackFromFile(
          pak_fd, ui::SCALE_FACTOR_100P);
    }

    base::i18n::SetICUDefaultLocale(locale);
    const std::string loaded_locale = locale;
#else
    const std::string loaded_locale =
        ResourceBundle::InitSharedInstanceWithLocale(locale, NULL);

    base::FilePath resources_pack_path;
    PathService::Get(chrome::FILE_RESOURCES_PACK, &resources_pack_path);
    ResourceBundle::GetSharedInstance().AddDataPackFromPath(
        resources_pack_path, ui::SCALE_FACTOR_NONE);
#endif
    CHECK(!loaded_locale.empty()) << "Locale could not be found for " <<
        locale;

It seems to me Linux should do something similar (re-using most of the Android code) and pass the .pak FD to the renderer. Does that sound like the right approach?

Pawel

[Roland McGrath]

That would be consistent with what we did for NaCl's IRT file
(nacl_*.nexe) on Linux, because of the auto-update issue.
NaClProcessHost::EarlyStartup opens the file. It's called from
ChromeBrowserMainParts::PreMainMessageLoopRunImpl.

[Antoine Labour]

On Wed, Sep 25, 2013 at 2:12 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

More into my investigation it seems that each renderer loads the files freshly from disk (I've changed log in data_pack.cc to FATAL):

[10052:10052:0925/140559:ERROR:memory_mapped_file.cc(51)] Couldn't open /usr/local/google/home/phajdan/chromium/src/out/Debug/chrome_100_percent.pak

[10052:10052:0925/140559:FATAL:data_pack.cc(78)] Failed to mmap datapack

 [0x555556b32da0] base::debug::StackTrace::StackTrace()

 [0x555556b6dd2b] logging::LogMessage::~LogMessage()

 [0x5555572c54e6] ui::DataPack::LoadFromPath()

 [0x5555572c9790] ui::ResourceBundle::AddDataPackFromPathInternal()

 [0x5555572c776b] ui::ResourceBundle::AddDataPackFromPath()

 [0x5555572cda4c] ui::ResourceBundle::LoadCommonResources()

 [0x5555572c740d] ui::ResourceBundle::InitSharedInstanceWithLocale()

 [0x555555d6f4cf] ChromeMainDelegate::PreSandboxStartup()

 [0x555556b16d48] content::ContentMainRunnerImpl::Initialize()

 [0x555556b154fe] content::ContentMain()

 [0x555555d6e09d] ChromeMain

 [0x555555d6e05c] main

 [0x7ffff1e3476d] __libc_start_main

 [0x555555d6df69] <unknown>

Mmh, this code should happen in the browser before the zygote is launched. The zygote process should inherit this. Are you not seeing the renderers using the zygote?

Antoine

[Paweł Hajdan, Jr.]

Oops, this was with --no-sandbox for debugging, I think that may have bypassed the zygote.

Still, I think the code I quoted earlier above pretty clearly loads from disk and not FD / memory.

Paweł

[Antoine Labour]

On Wed, Sep 25, 2013 at 4:31 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Oops, this was with --no-sandbox for debugging, I think that may have bypassed the zygote.

Still, I think the code I quoted earlier above pretty clearly loads from disk and not FD / memory.

Right, but it should happen in the browser process, before we fork the zygote, i.e. what was meant by the consensus "this should happen early". It opens the file (as a fd) and mmaps it. This stays forever in the process, and the children that don't exec (i.e. zygote, renderers). If the file is deleted, nothing bad should happen.

Antoine

[Paweł Hajdan, Jr.]

Alright, so the plot thickens.

I can get the renderers to work in face of deleted/moved resources with https://codereview.chromium.org/24840002 .

Now the service processes (http://www.chromium.org/developers/design-documents/service-processes) and utility process and maybe more (https://code.google.com/p/chromium/issues/detail?id=22703) are not using the zygote. I don't see a way for them to behave well in this scenario. This bug is affecting Chrome Linux since the beginning, and so I don't think (after fixing the most visible renderer issue I think) this should block beta release.

Paweł