Fix a bug where an extension's icon was not being properly updated.

Fix a bug where an extension's icon was not being properly updated.

A query string, generated when an extension is loaded or reloaded, is appended to the extension icon url to prevent it from using the cached image when it may have been changed.

BUG=159302

[dhnishi (use Chromium), 2013-06-19 22:41:57]

Hello!

I am a new coder with the Extensions group at Cal Poly San Luis Obispo. I picked
up this issue as my first bug and this is my first submit. 

There was a previous fix that was reverted because it caused all extensions to
reload their images. This fix is similar, but it only reloads the images if the
extension was actually reloaded, rather than reloading all of them.

Please review this when you have some time.

Thank you very much,
Daniel Nishi

[not at google - send to devlin, 2013-06-20 15:06:30]

I'm going to divert this to Jeffrey actually, despite me reviewing the last one
I think he'll have a better idea of what's going on.

Jeffrey - the extension icons in chrome://extensions are being cached too long,
so that if an extension changes its icon then it doesn't update. You can
actually see this phenomenon in the devtools.

Daniel - the thing with the image ID, in fact the whole appending a get request
to kill caching, still seems unnecessary to me. Kind of a hack. It seems like we
could, should in some way, be able to invalidate the browser cache for that
individual image, or something along those lines. This is where I hope Jeffrey
knows what's going on.

Anyhow - welcome!

[dhnishi (use Chromium), 2013-06-24 18:20:08]

Hello Jeffrey,

I was wondering if you could take a look at this. I couldn't figure out how
Chrome handles cache invalidation from the little bits of knowledge I got from
looking through the code base -- do you think that a cache invalidation solution
would be viable?

Thanks,
Daniel

On 2013/06/20 15:06:30, kalman - OOO UNTIL 7-1 wrote:
> I'm going to divert this to Jeffrey actually, despite me reviewing the last
one
> I think he'll have a better idea of what's going on.
> 
> Jeffrey - the extension icons in chrome://extensions are being cached too
long,
> so that if an extension changes its icon then it doesn't update. You can
> actually see this phenomenon in the devtools.
> 
> Daniel - the thing with the image ID, in fact the whole appending a get
request
> to kill caching, still seems unnecessary to me. Kind of a hack. It seems like
we
> could, should in some way, be able to invalidate the browser cache for that
> individual image, or something along those lines. This is where I hope Jeffrey
> knows what's going on.
> 
> Anyhow - welcome!

[Jeffrey Yasskin, 2013-06-24 23:02:53]

On 2013/06/24 18:20:08, Daniel Nishi wrote:
> Hello Jeffrey,
> 
> I was wondering if you could take a look at this. I couldn't figure out how
> Chrome handles cache invalidation from the little bits of knowledge I got from
> looking through the code base -- do you think that a cache invalidation
solution
> would be viable?

This might be as simple as overriding URLDataSource::AllowCaching() in
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui...,
or you might have to change where ExtensionIconSource pulls its data from in
order to prevent it from always loading an unpacked-extension's icon from disk.

It'd be good to find a way to test this, too.

> Thanks,
> Daniel
> 
> On 2013/06/20 15:06:30, kalman - OOO UNTIL 7-1 wrote:
> > I'm going to divert this to Jeffrey actually, despite me reviewing the last
> one
> > I think he'll have a better idea of what's going on.
> > 
> > Jeffrey - the extension icons in chrome://extensions are being cached too
> long,
> > so that if an extension changes its icon then it doesn't update. You can
> > actually see this phenomenon in the devtools.
> > 
> > Daniel - the thing with the image ID, in fact the whole appending a get
> request
> > to kill caching, still seems unnecessary to me. Kind of a hack. It seems
like
> we
> > could, should in some way, be able to invalidate the browser cache for that
> > individual image, or something along those lines. This is where I hope
Jeffrey
> > knows what's going on.
> > 
> > Anyhow - welcome!

[dhnishi (use Chromium), 2013-06-25 17:03:05]

It looks like the URLDataSource's AllowCaching applies to all of the responses
that come from the data source. If it's set to true, then nothing gets updated,
if it is set to false, all of the extensions' images get updated -- doesn't look
like we can get this to work for an individual response from it. 

I played around with the ExtensionIconSource and found out that most of the
functions inside of it seems to only run if Chrome makes an actual request at
the internal extension-icon location, rather than using the cache. Looks like
even if we changed where it was pulling its data, it would still only get the
call once unless we had another method of invalidating the cache. Most of the
URLDataSource bits look dependent on getting called by a request for data, and
if the browser never requests the data again then it would only get run once. We
could technically take the JS route on the static GURL parts of the
ExtensionIconSource, but that solution doesn't seem much less hack-like.

My search for other cache invalidation methods got me to http_cache, which
interfaces with the disk_cache backend, but I don't think that the extension
code should be able to access it to do a direct invalidation of a cached GET
call. 

I put together a solution going that was making query strings based on the last
couple digits of the extension's ptr in the ExtensionIconSource's URL returning,
because reloading would make a whole new Extension and therefore a whole new
memory location. This solution seemed very hack-like, too, so I didn't even
bother uploading that cl.

On 2013/06/24 23:02:53, Jeffrey Yasskin wrote:
> On 2013/06/24 18:20:08, Daniel Nishi wrote:
> > Hello Jeffrey,
> > 
> > I was wondering if you could take a look at this. I couldn't figure out how
> > Chrome handles cache invalidation from the little bits of knowledge I got
from
> > looking through the code base -- do you think that a cache invalidation
> solution
> > would be viable?
> 
> This might be as simple as overriding URLDataSource::AllowCaching() in
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui...,
> or you might have to change where ExtensionIconSource pulls its data from in
> order to prevent it from always loading an unpacked-extension's icon from
disk.
> 
> It'd be good to find a way to test this, too.
> 
> > Thanks,
> > Daniel
> > 
> > On 2013/06/20 15:06:30, kalman - OOO UNTIL 7-1 wrote:
> > > I'm going to divert this to Jeffrey actually, despite me reviewing the
last
> > one
> > > I think he'll have a better idea of what's going on.
> > > 
> > > Jeffrey - the extension icons in chrome://extensions are being cached too
> > long,
> > > so that if an extension changes its icon then it doesn't update. You can
> > > actually see this phenomenon in the devtools.
> > > 
> > > Daniel - the thing with the image ID, in fact the whole appending a get
> > request
> > > to kill caching, still seems unnecessary to me. Kind of a hack. It seems
> like
> > we
> > > could, should in some way, be able to invalidate the browser cache for
that
> > > individual image, or something along those lines. This is where I hope
> Jeffrey
> > > knows what's going on.
> > > 
> > > Anyhow - welcome!

[Jeffrey Yasskin, 2013-06-25 18:52:10]

Since extension icons can change at any time (in response to the user
clicking 'reload'), and we want that change to show up instantly, we
shouldn't be caching them. There might be a way to emulate a
Shift-Refresh on just a few images (which would force a cache
invalidation), but I don't think that's the right approach.

Not-caching all extension icons may cause the problem Hokein.Wu
pointed out at http://crbug.com/159302#c5, but we should fix that
problem some other way, probably by having the ExtensionIconSource
serve from the loaded extension rather than its unpacked source on
disk.

On Tue, Jun 25, 2013 at 10:03 AM,  <DHNishi@gmail.com> wrote:
> It looks like the URLDataSource's AllowCaching applies to all of the
> responses
> that come from the data source. If it's set to true, then nothing gets
> updated,
> if it is set to false, all of the extensions' images get updated -- doesn't
> look
> like we can get this to work for an individual response from it.
>
> I played around with the ExtensionIconSource and found out that most of the
> functions inside of it seems to only run if Chrome makes an actual request
> at
> the internal extension-icon location, rather than using the cache. Looks
> like
> even if we changed where it was pulling its data, it would still only get
> the
> call once unless we had another method of invalidating the cache. Most of
> the
> URLDataSource bits look dependent on getting called by a request for data,
> and
> if the browser never requests the data again then it would only get run
> once. We
> could technically take the JS route on the static GURL parts of the
> ExtensionIconSource, but that solution doesn't seem much less hack-like.
>
> My search for other cache invalidation methods got me to http_cache, which
> interfaces with the disk_cache backend, but I don't think that the extension
> code should be able to access it to do a direct invalidation of a cached GET
> call.
>
> I put together a solution going that was making query strings based on the
> last
> couple digits of the extension's ptr in the ExtensionIconSource's URL
> returning,
> because reloading would make a whole new Extension and therefore a whole new
> memory location. This solution seemed very hack-like, too, so I didn't even
> bother uploading that cl.
>
>
> On 2013/06/24 23:02:53, Jeffrey Yasskin wrote:
>>
>> On 2013/06/24 18:20:08, Daniel Nishi wrote:
>> > Hello Jeffrey,
>> >
>> > I was wondering if you could take a look at this. I couldn't figure out
>> > how
>> > Chrome handles cache invalidation from the little bits of knowledge I
>> > got
>
> from
>>
>> > looking through the code base -- do you think that a cache invalidation
>> solution
>> > would be viable?
>
>
>> This might be as simple as overriding URLDataSource::AllowCaching() in
>
>
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui...,
>>
>> or you might have to change where ExtensionIconSource pulls its data from
>> in
>> order to prevent it from always loading an unpacked-extension's icon from
>
> disk.
>
>> It'd be good to find a way to test this, too.
>
>
>> > Thanks,
>> > Daniel
>> >
>> > On 2013/06/20 15:06:30, kalman - OOO UNTIL 7-1 wrote:
>> > > I'm going to divert this to Jeffrey actually, despite me reviewing the
>
> last
>>
>> > one
>> > > I think he'll have a better idea of what's going on.
>> > >
>> > > Jeffrey - the extension icons in chrome://extensions are being cached
>> > > too
>> > long,
>> > > so that if an extension changes its icon then it doesn't update. You
>> > > can
>> > > actually see this phenomenon in the devtools.
>> > >
>> > > Daniel - the thing with the image ID, in fact the whole appending a
>> > > get
>> > request
>> > > to kill caching, still seems unnecessary to me. Kind of a hack. It
>> > > seems
>> like
>> > we
>> > > could, should in some way, be able to invalidate the browser cache for
>
> that
>>
>> > > individual image, or something along those lines. This is where I hope
>> Jeffrey
>> > > knows what's going on.
>> > >
>> > > Anyhow - welcome!
>
>
>
>
> https://codereview.chromium.org/16831012/

[dhnishi (use Chromium), 2013-06-26 18:08:00]

It looks like ExtensionIconSource's image fetching aspects only are run when it
receives the GET request for the chrome://extension-icon and it parses that
path. It seems like even if we changed how it loaded the image, it still would
have the problem, because that changed serving code still wouldn't get run
because the browser isn't requesting the image.

On 2013/06/25 18:52:10, Jeffrey Yasskin wrote:
> Since extension icons can change at any time (in response to the user
> clicking 'reload'), and we want that change to show up instantly, we
> shouldn't be caching them. There might be a way to emulate a
> Shift-Refresh on just a few images (which would force a cache
> invalidation), but I don't think that's the right approach.
> 
> Not-caching all extension icons may cause the problem Hokein.Wu
> pointed out at http://crbug.com/159302#c5, but we should fix that
> problem some other way, probably by having the ExtensionIconSource
> serve from the loaded extension rather than its unpacked source on
> disk.
> 
> On Tue, Jun 25, 2013 at 10:03 AM,  <mailto:DHNishi@gmail.com> wrote:
> > It looks like the URLDataSource's AllowCaching applies to all of the
> > responses
> > that come from the data source. If it's set to true, then nothing gets
> > updated,
> > if it is set to false, all of the extensions' images get updated -- doesn't
> > look
> > like we can get this to work for an individual response from it.
> >
> > I played around with the ExtensionIconSource and found out that most of the
> > functions inside of it seems to only run if Chrome makes an actual request
> > at
> > the internal extension-icon location, rather than using the cache. Looks
> > like
> > even if we changed where it was pulling its data, it would still only get
> > the
> > call once unless we had another method of invalidating the cache. Most of
> > the
> > URLDataSource bits look dependent on getting called by a request for data,
> > and
> > if the browser never requests the data again then it would only get run
> > once. We
> > could technically take the JS route on the static GURL parts of the
> > ExtensionIconSource, but that solution doesn't seem much less hack-like.
> >
> > My search for other cache invalidation methods got me to http_cache, which
> > interfaces with the disk_cache backend, but I don't think that the extension
> > code should be able to access it to do a direct invalidation of a cached GET
> > call.
> >
> > I put together a solution going that was making query strings based on the
> > last
> > couple digits of the extension's ptr in the ExtensionIconSource's URL
> > returning,
> > because reloading would make a whole new Extension and therefore a whole new
> > memory location. This solution seemed very hack-like, too, so I didn't even
> > bother uploading that cl.
> >
> >
> > On 2013/06/24 23:02:53, Jeffrey Yasskin wrote:
> >>
> >> On 2013/06/24 18:20:08, Daniel Nishi wrote:
> >> > Hello Jeffrey,
> >> >
> >> > I was wondering if you could take a look at this. I couldn't figure out
> >> > how
> >> > Chrome handles cache invalidation from the little bits of knowledge I
> >> > got
> >
> > from
> >>
> >> > looking through the code base -- do you think that a cache invalidation
> >> solution
> >> > would be viable?
> >
> >
> >> This might be as simple as overriding URLDataSource::AllowCaching() in
> >
> >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ui...,
> >>
> >> or you might have to change where ExtensionIconSource pulls its data from
> >> in
> >> order to prevent it from always loading an unpacked-extension's icon from
> >
> > disk.
> >
> >> It'd be good to find a way to test this, too.
> >
> >
> >> > Thanks,
> >> > Daniel
> >> >
> >> > On 2013/06/20 15:06:30, kalman - OOO UNTIL 7-1 wrote:
> >> > > I'm going to divert this to Jeffrey actually, despite me reviewing the
> >
> > last
> >>
> >> > one
> >> > > I think he'll have a better idea of what's going on.
> >> > >
> >> > > Jeffrey - the extension icons in chrome://extensions are being cached
> >> > > too
> >> > long,
> >> > > so that if an extension changes its icon then it doesn't update. You
> >> > > can
> >> > > actually see this phenomenon in the devtools.
> >> > >
> >> > > Daniel - the thing with the image ID, in fact the whole appending a
> >> > > get
> >> > request
> >> > > to kill caching, still seems unnecessary to me. Kind of a hack. It
> >> > > seems
> >> like
> >> > we
> >> > > could, should in some way, be able to invalidate the browser cache for
> >
> > that
> >>
> >> > > individual image, or something along those lines. This is where I hope
> >> Jeffrey
> >> > > knows what's going on.
> >> > >
> >> > > Anyhow - welcome!
> >
> >
> >
> >
> > https://codereview.chromium.org/16831012/

[Jeffrey Yasskin, 2013-06-27 00:04:30]

On Wed, Jun 26, 2013 at 11:08 AM,  <DHNishi@gmail.com> wrote:
> It looks like ExtensionIconSource's image fetching aspects only are run when
> it
> receives the GET request for the chrome://extension-icon and it parses that
> path. It seems like even if we changed how it loaded the image, it still
> would
> have the problem, because that changed serving code still wouldn't get run
> because the browser isn't requesting the image.

You're right; it looks like most techniques of loading an image don't
refresh from the server even if the server says not to cache the data.
I'd still rather not complicate the rest of the extensions code to fix
this bug. I've replied on the bug suggesting that we just take
cduvall's change, although it might make sense to use Date.now() as
the uniquifying value instead of maintaining a counter.