Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Brady Eidson
Hi all,

The IconDatabase in WebCore is the source of crashes, spins, and complexity. Additionally it’s not flexible enough to acknowledge that there’s multiple types of site icons in use on the modern web, nor to adapt to the embedding client’s need for customization.

I recently introduced the “_WKIconLoadingDelegate” model to WebKit2Cocoa.

WebCore gathers all of the candidate icon URLs and asks the embedding app for each one whether or not it wants to load them.
If the app says yes, the icon will be loaded as a subresource in the current document and the data will be handed off to the client.

From Apple’s perspective:

The new model is powerful and flexible enough that Safari has adopted it.
In WebKit1, the “WebIconDatabase” class was never API and is currently unused.
In WebKit2, the C-API support was never API and is currently unused.

Therefore we intend to remove the current WebCore IconDatabase from the project soon.

Starting in on this task, I of course noticed GTK’s API has exposed “WebKitFaviconDatabase”

Is that something that’s published API and that is used?
If not, I can get rid of it right now

If so, then I need a GTK maintainer to come up with a plan soon.

The icon load delegate mechanism is powerful enough to rebuild an IconDatabase on top of, so if GTK needs to keep this API functional they can do so - maintaining the actual IconDatabase code themselves up in their API layer.

Thanks,
~Brady
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Maciej Stachowiak

This is slightly tangential, but a comment on the model: it doesn't seem like there's a way for clients to check what range of icons are available and only then choose which to load. Even though Safari may not have needed this to move over, if you wanted to do something rigorous like load the closest available size to what you need or else a scalable format, there's no way to do that without potentially loading icons you don't need. While Safari hasn't done this, it might be useful if Safari's various places that display icons are ever made more consistent and coherent.

I can see that there needs to be some per-icon notification, since <link> elements can be added after the fact, and also incremental parsing is a thing. So one notification that tells you all icons wouldn't cut it.

But separating the loading mechanism from the notification, plus adding a notification that the <head> section is done parsing, could allow avoidance of unnecessary loads. In addition, there could be other useful future uses of a mechanism to properly load a resource as if it was being loaded by the page. So it would be nice to decouple this from the notification of discovering an icon link.


Another minor comment: it seems like this new API returns raw data. It seems like the native way to use this would result in running untrusted data from the network through image decoders outside the Web Process sandbox. Do we have a way to avoid that?


Asking these questions because if this is to be the One True Model of icon loading going forward, we should try to make sure the design is future-proof.


Regards,
Maciej


> On Jun 15, 2017, at 5:11 PM, Brady Eidson <[hidden email]> wrote:
>
> Hi all,
>
> The IconDatabase in WebCore is the source of crashes, spins, and complexity. Additionally it’s not flexible enough to acknowledge that there’s multiple types of site icons in use on the modern web, nor to adapt to the embedding client’s need for customization.
>
> I recently introduced the “_WKIconLoadingDelegate” model to WebKit2Cocoa.
>
> WebCore gathers all of the candidate icon URLs and asks the embedding app for each one whether or not it wants to load them.
> If the app says yes, the icon will be loaded as a subresource in the current document and the data will be handed off to the client.
>
> From Apple’s perspective:
>
> The new model is powerful and flexible enough that Safari has adopted it.
> In WebKit1, the “WebIconDatabase” class was never API and is currently unused.
> In WebKit2, the C-API support was never API and is currently unused.
>
> Therefore we intend to remove the current WebCore IconDatabase from the project soon.
>
> Starting in on this task, I of course noticed GTK’s API has exposed “WebKitFaviconDatabase”
>
> Is that something that’s published API and that is used?
> If not, I can get rid of it right now
>
> If so, then I need a GTK maintainer to come up with a plan soon.
>
> The icon load delegate mechanism is powerful enough to rebuild an IconDatabase on top of, so if GTK needs to keep this API functional they can do so - maintaining the actual IconDatabase code themselves up in their API layer.
>
> Thanks,
> ~Brady
> _______________________________________________
> webkit-dev mailing list
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Michael Catanzaro
In reply to this post by Brady Eidson
On Thu, Jun 15, 2017 at 7:11 PM, Brady Eidson <[hidden email]> wrote:
> Hi all,
>
> The IconDatabase in WebCore is the source of crashes, spins, and
> complexity.

Yup. It's very unreliable for GTK. It's the source of
difficult-to-understand memory leaks, difficult-to-reproduce crashes,
and, more commonly, garbled or gibberish icons. I'm guessing a thread
safety issue we haven't been able to figure out.

> Starting in on this task, I of course noticed GTK’s API has exposed
> “WebKitFaviconDatabase”
>
> Is that something that’s published API and that is used?
> If not, I can get rid of it right now

Yes it's published API, and yes we need to keep it working.

> If so, then I need a GTK maintainer to come up with a plan soon.
>
> The icon load delegate mechanism is powerful enough to rebuild an
> IconDatabase on top of, so if GTK needs to keep this API functional
> they can do so - maintaining the actual IconDatabase code themselves
> up in their API layer.

I haven't looked at this closely, but I strongly suspect we could
reimplement the WebKitFaviconDatabase API on top of your icon load
delegate mechanism like you suggest. It's probably going to be better
for us in the long run anyway, because our current code is just not
reliable.

> On Thu, Jun 15, 2017 at 7:27 PM, Maciej Stachowiak <[hidden email]>
> wrote:
> This is slightly tangential, but a comment on the model: it doesn't
> seem like there's a way for clients to check what range of icons are
> available and only then choose which to load. Even though Safari may
> not have needed this to move over, if you wanted to do something
> rigorous like load the closest available size to what you need or
> else a scalable format, there's no way to do that without potentially
> loading icons you don't need. While Safari hasn't done this, it might
> be useful if Safari's various places that display icons are ever made
> more consistent and coherent.

Something like this could be useful for Epiphany, where I think we just
want a couple specific icon sizes.

One issue we've noticed is there are many different nonstandard ways
that websites use to find icons. [1] details many of these: the
Microsoft way, the Apple way, the OpenGraph way... and even then the
icons can be put in different places... is this the problem Brady's
code is designed to solve?

Michael

[1] https://ctrl.blog/entry/gnome-web-app-icons

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Brady Eidson
In reply to this post by Maciej Stachowiak
Slight reordering.

> On Jun 15, 2017, at 5:27 PM, Maciej Stachowiak <[hidden email]> wrote:
>
>
> This is slightly tangential, but a comment on the model: it doesn't seem like there's a way for clients to check what range of icons are available and only then choose which to load. Even though Safari may not have needed this to move over, if you wanted to do something rigorous like load the closest available size to what you need or else a scalable format, there's no way to do that without potentially loading icons you don't need. While Safari hasn't done this, it might be useful if Safari's various places that display icons are ever made more consistent and coherent.

This was discussed when implementing the model for Safari, which was admittedly done quickly.

While Safari didn't need what you suggest right now, they agreed they might need it in the future. Since the load decision request has a completion callback, and since it's a known implementation detail that they will all come in quick succession, it was accepted that the current model could work for "choosing the right icon of all choices" in a pinch.

> I can see that there needs to be some per-icon notification, since <link> elements can be added after the fact, and also incremental parsing is a thing. So one notification that tells you all icons wouldn't cut it.

Right - The "just one icon" notification was the bare minimum because of this need.
We can definitely add a greater API surface to deliver a bulk "here's all the icons in the <head>" in addition to the individual icon notification.

> Another minor comment: it seems like this new API returns raw data. It seems like the native way to use this would result in running untrusted data from the network through image decoders outside the Web Process sandbox. Do we have a way to avoid that?

This came up while implementing it for Safari, too. In practice we didn't decode icons out-of-process before so this model was not a regression. I see value in offering this, but it's also something conscientious clients can do on their own with the raw data.

Also...

> But separating the loading mechanism from the notification, plus adding a notification that the <head> section is done parsing, could allow avoidance of unnecessary loads. In addition, there could be other useful future uses of a mechanism to properly load a resource as if it was being loaded by the page. So it would be nice to decouple this from the notification of discovering an icon link.

That's the full-on tangent!

As you know, "load an arbitrary subresource in the context of the page" has come up plenty of times over the history of the WK API, so it does seem like there's value here.
Offering that, however, does seem to make the "decode the icon image out of process" less of a fit with a general purpose API.

> Asking these questions because if this is to be the One True Model of icon loading going forward, we should try to make sure the design is future-proof.

Yes, these are all great questions to ask.

Everything that's been brought up so far only really touches the API surface (Do we batch together icon load decision requests? Do we give an out-of-process decoded image instead of raw data? Do we offer an API for loading arbitrary subresource?)

Speaking with understanding of what's implemented today and what would be necessary to make any or all of these changes, I know that the current mechanism is a solid base on which these ideas can easily be built.

Thanks,
 Brady

>
>
> Regards,
> Maciej
>
>
>> On Jun 15, 2017, at 5:11 PM, Brady Eidson <[hidden email]> wrote:
>>
>> Hi all,
>>
>> The IconDatabase in WebCore is the source of crashes, spins, and complexity. Additionally it’s not flexible enough to acknowledge that there’s multiple types of site icons in use on the modern web, nor to adapt to the embedding client’s need for customization.
>>
>> I recently introduced the “_WKIconLoadingDelegate” model to WebKit2Cocoa.
>>
>> WebCore gathers all of the candidate icon URLs and asks the embedding app for each one whether or not it wants to load them.
>> If the app says yes, the icon will be loaded as a subresource in the current document and the data will be handed off to the client.
>>
>> From Apple’s perspective:
>>
>> The new model is powerful and flexible enough that Safari has adopted it.
>> In WebKit1, the “WebIconDatabase” class was never API and is currently unused.
>> In WebKit2, the C-API support was never API and is currently unused.
>>
>> Therefore we intend to remove the current WebCore IconDatabase from the project soon.
>>
>> Starting in on this task, I of course noticed GTK’s API has exposed “WebKitFaviconDatabase”
>>
>> Is that something that’s published API and that is used?
>> If not, I can get rid of it right now
>>
>> If so, then I need a GTK maintainer to come up with a plan soon.
>>
>> The icon load delegate mechanism is powerful enough to rebuild an IconDatabase on top of, so if GTK needs to keep this API functional they can do so - maintaining the actual IconDatabase code themselves up in their API layer.
>>
>> Thanks,
>> ~Brady
>> _______________________________________________
>> webkit-dev mailing list
>> [hidden email]
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Brady Eidson
In reply to this post by Michael Catanzaro

> On Jun 15, 2017, at 7:27 PM, Michael Catanzaro <[hidden email]> wrote:
>
> One issue we've noticed is there are many different nonstandard ways that websites use to find icons. [1] details many of these: the Microsoft way, the Apple way, the OpenGraph way... and even then the icons can be put in different places... is this the problem Brady's code is designed to solve?

The WebCore::IconDatabase was extremely limited.

It would look at only the last <link rel="icon"> or <link rel="shortcut icon"> (which are standardized in HTML5) and consider it "the one icon" for a page.
If the <head> specified none, it would consider the default /favicon.ico the one icon.
And all it could load/store is that one icon.

The new mechanism already expands upon this by allowing for *each* <link> icon to be considered individually.
It also expands upon this by having built-in support for apple-touch and apple-touch-precomposed icons.

There's nothing stopping us from adding support for all of these icon types with only small changes to WebCore. Of the list in that blog post some might be considered relevant to Apple, others probably not - But LinkIconCollector is a tidy self-contained class that can easily have per-platform implementations.

tl;dr - Yes, this is basically the problem the new mechanism solves.

Thanks,
 Brady
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Carlos Garcia Campos-7
In reply to this post by Brady Eidson
El jue, 15-06-2017 a las 17:11 -0700, Brady Eidson escribió:

> Hi all,
>
> The IconDatabase in WebCore is the source of crashes, spins, and
> complexity. Additionally it’s not flexible enough to acknowledge that
> there’s multiple types of site icons in use on the modern web, nor to
> adapt to the embedding client’s need for customization.
>
> I recently introduced the “_WKIconLoadingDelegate” model to
> WebKit2Cocoa.
>
> WebCore gathers all of the candidate icon URLs and asks the embedding
> app for each one whether or not it wants to load them.
> If the app says yes, the icon will be loaded as a subresource in the
> current document and the data will be handed off to the client.

Do we have any cache here? would we use the disk cache like with any
other subresource?

> From Apple’s perspective:
>
> The new model is powerful and flexible enough that Safari has adopted
> it.
> In WebKit1, the “WebIconDatabase” class was never API and is
> currently unused.
> In WebKit2, the C-API support was never API and is currently unused.
>
> Therefore we intend to remove the current WebCore IconDatabase from
> the project soon.

This is great news.

> Starting in on this task, I of course noticed GTK’s API has exposed
> “WebKitFaviconDatabase”
>
> Is that something that’s published API and that is used?

Yes, it is part of our public API, so we need to keep backwards
compatibility.

> If not, I can get rid of it right now
>
> If so, then I need a GTK maintainer to come up with a plan soon.

How soon is soon? :-) We are approaching the end of our release cycle,
it would be good for us if we could do any changes related to this
after we branch for 2.18. Of course I can branch earlier if needed.
According to our schedule we should branch the first week of August. Is
that late?

> The icon load delegate mechanism is powerful enough to rebuild an
> IconDatabase on top of, so if GTK needs to keep this API functional
> they can do so - maintaining the actual IconDatabase code themselves
> up in their API layer.

I have to look at it in detail. If I can use the new mechanism to
reimplement WebKitFaviconDatabase, I'll definitely do that. But if
there's no longer a database of favicons, I wouldn't mind to deprecate
it and provide a new better API. But in any case, I need to keep
WebKitFaviconDatabase working, whether using new way or moving the
current IconDatabase to wk2gtk.

> Thanks,
> ~Brady
> _______________________________________________
> webkit-dev mailing list
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Michael Catanzaro
On Fri, Jun 16, 2017 at 12:59 AM, Carlos Garcia Campos
<[hidden email]> wrote:
> Do we have any cache here? would we use the disk cache like with any
> other subresource?

Tangent: we currently have a bug where the mixed content warning is not
issued if a favicon was downloaded in an insecure context, cached, and
then displayed in a secure context. In practice this means you get a
mixed content warning only the first time you visit a secure website
with an insecure favicon, but never again subsequently, hiding the bug
from website developers. That's bad!

https://bugs.webkit.org/show_bug.cgi?id=142340

Michael

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Brady Eidson
In reply to this post by Brady Eidson

> On Jun 15, 2017, at 10:54 PM, Carlos Garcia Campos <[hidden email]> wrote:
>
> El jue, 15-06-2017 a las 17:11 -0700, Brady Eidson escribió:
>> Hi all,
>>
>> The IconDatabase in WebCore is the source of crashes, spins, and
>> complexity. Additionally it’s not flexible enough to acknowledge that
>> there’s multiple types of site icons in use on the modern web, nor to
>> adapt to the embedding client’s need for customization.
>>
>> I recently introduced the “_WKIconLoadingDelegate” model to
>> WebKit2Cocoa.
>>
>> WebCore gathers all of the candidate icon URLs and asks the embedding
>> app for each one whether or not it wants to load them.
>> If the app says yes, the icon will be loaded as a subresource in the
>> current document and the data will be handed off to the client.
>
> Do we have any cache here? would we use the disk cache like with any
> other subresource?

As noted, they are loaded as subresources in the current document.
As such, they go through the memory cache and disk cache.

Note: IconDatabase loads actually worked the same way.

>> If not, I can get rid of it right now
>>
>> If so, then I need a GTK maintainer to come up with a plan soon.
>
> How soon is soon? :-) We are approaching the end of our release cycle,
> it would be good for us if we could do any changes related to this
> after we branch for 2.18. Of course I can branch earlier if needed.
> According to our schedule we should branch the first week of August. Is
> that late?

That’s much later than the soon I was targeting.

Hoping to do this by the end of June.

>> The icon load delegate mechanism is powerful enough to rebuild an
>> IconDatabase on top of, so if GTK needs to keep this API functional
>> they can do so - maintaining the actual IconDatabase code themselves
>> up in their API layer.
>
> I have to look at it in detail. If I can use the new mechanism to
> reimplement WebKitFaviconDatabase, I'll definitely do that. But if
> there's no longer a database of favicons, I wouldn't mind to deprecate
> it and provide a new better API. But in any case, I need to keep
> WebKitFaviconDatabase working, whether using new way or moving the
> current IconDatabase to wk2gtk.

The new mechanism definitely provides all of the signaling necessary to reimplement the current IconDatabase, but a little plumbing will be involved.

Thanks,
~Brady

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Maciej Stachowiak
In reply to this post by Brady Eidson


> On Jun 15, 2017, at 8:01 PM, Brady Eidson <[hidden email]> wrote:
>
> Slight reordering.
>
>> On Jun 15, 2017, at 5:27 PM, Maciej Stachowiak <[hidden email]> wrote:
>>
>>
>> This is slightly tangential, but a comment on the model: it doesn't seem like there's a way for clients to check what range of icons are available and only then choose which to load. Even though Safari may not have needed this to move over, if you wanted to do something rigorous like load the closest available size to what you need or else a scalable format, there's no way to do that without potentially loading icons you don't need. While Safari hasn't done this, it might be useful if Safari's various places that display icons are ever made more consistent and coherent.
>
> This was discussed when implementing the model for Safari, which was admittedly done quickly.
>
> While Safari didn't need what you suggest right now, they agreed they might need it in the future. Since the load decision request has a completion callback, and since it's a known implementation detail that they will all come in quick succession, it was accepted that the current model could work for "choosing the right icon of all choices" in a pinch.

How are you supposed to do this? Save all the completion handlers indefinitely? I can sort of see how that could work, but if the completion handler is intended to be used at a possibly later time than the shouldLoadIconWithParameters: method, it's kind of weird that it's a callback at all.

>
>> I can see that there needs to be some per-icon notification, since <link> elements can be added after the fact, and also incremental parsing is a thing. So one notification that tells you all icons wouldn't cut it.
>
> Right - The "just one icon" notification was the bare minimum because of this need.
> We can definitely add a greater API surface to deliver a bulk "here's all the icons in the <head>" in addition to the individual icon notification.
>
>> Another minor comment: it seems like this new API returns raw data. It seems like the native way to use this would result in running untrusted data from the network through image decoders outside the Web Process sandbox. Do we have a way to avoid that?
>
> This came up while implementing it for Safari, too. In practice we didn't decode icons out-of-process before so this model was not a regression. I see value in offering this, but it's also something conscientious clients can do on their own with the raw data.

It's not that easy for an arbitrary macOS app to make their own tightly sandboxed service for this, and probably impossible on iOS. So if we made this interface into public API, there wouldn't be a way for most clients to do the right thing.

>
> Also...
>
>> But separating the loading mechanism from the notification, plus adding a notification that the <head> section is done parsing, could allow avoidance of unnecessary loads. In addition, there could be other useful future uses of a mechanism to properly load a resource as if it was being loaded by the page. So it would be nice to decouple this from the notification of discovering an icon link.
>
> That's the full-on tangent!
>
> As you know, "load an arbitrary subresource in the context of the page" has come up plenty of times over the history of the WK API, so it does seem like there's value here.
> Offering that, however, does seem to make the "decode the icon image out of process" less of a fit with a general purpose API.

Yeah, we'd either have to provide a generic image decoding service or make a generic loading facility that's capable of vending decoded image data instead of raw data, which both seem a bit inelegant.

>> Asking these questions because if this is to be the One True Model of icon loading going forward, we should try to make sure the design is future-proof.
>
> Yes, these are all great questions to ask.
>
> Everything that's been brought up so far only really touches the API surface (Do we batch together icon load decision requests? Do we give an out-of-process decoded image instead of raw data? Do we offer an API for loading arbitrary subresource?)
>
> Speaking with understanding of what's implemented today and what would be necessary to make any or all of these changes, I know that the current mechanism is a solid base on which these ideas can easily be built.

OK, as long as we still have room to address these issues before it becomes locked in as public API, then I am fine with not addressing these issues for now.

Regards,
Maciej



_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Brady Eidson

On Jun 16, 2017, at 11:11 AM, Maciej Stachowiak <[hidden email]> wrote:
This is slightly tangential, but a comment on the model: it doesn't seem like there's a way for clients to check what range of icons are available and only then choose which to load. Even though Safari may not have needed this to move over, if you wanted to do something rigorous like load the closest available size to what you need or else a scalable format, there's no way to do that without potentially loading icons you don't need. While Safari hasn't done this, it might be useful if Safari's various places that display icons are ever made more consistent and coherent.

This was discussed when implementing the model for Safari, which was admittedly done quickly.

While Safari didn't need what you suggest right now, they agreed they might need it in the future. Since the load decision request has a completion callback, and since it's a known implementation detail that they will all come in quick succession, it was accepted that the current model could work for "choosing the right icon of all choices" in a pinch.

How are you supposed to do this? Save all the completion handlers indefinitely? I can sort of see how that could work, but if the completion handler is intended to be used at a possibly later time than the shouldLoadIconWithParameters: method, it's kind of weird that it's a callback at all.

It definitely needs to be asynchronous because loading decisions can (often? always?) depend on i/o to a database.

Scenario:

1 - WebKit says “There’s a favicon of size 64x64 from URL “http://foo.com/favicon.ico” - Do you want me to load it?
2 - Client saves off that decision handler and queries its database to see if it wants that icon. (Do I have this one already? Do I have it but in a smaller size? Is my icon weeks old and therefore I want a refreshed one?)
3 - After the i/o for the database operation, the client decides it does (or does not) want this icon, and calls the completion handler.


I can see that there needs to be some per-icon notification, since <link> elements can be added after the fact, and also incremental parsing is a thing. So one notification that tells you all icons wouldn't cut it.

Right - The "just one icon" notification was the bare minimum because of this need.
We can definitely add a greater API surface to deliver a bulk "here's all the icons in the <head>" in addition to the individual icon notification.

Another minor comment: it seems like this new API returns raw data. It seems like the native way to use this would result in running untrusted data from the network through image decoders outside the Web Process sandbox. Do we have a way to avoid that?

This came up while implementing it for Safari, too. In practice we didn't decode icons out-of-process before so this model was not a regression. I see value in offering this, but it's also something conscientious clients can do on their own with the raw data.

It's not that easy for an arbitrary macOS app to make their own tightly sandboxed service for this, and probably impossible on iOS. So if we made this interface into public API, there wouldn't be a way for most clients to do the right thing.

There’s a *lot* of questions to answer before we even consider making this API.

Considering nothing related to the icon database has ever been API, there wasn’t a compelling reason to answer those questions this time around.

But separating the loading mechanism from the notification, plus adding a notification that the <head> section is done parsing, could allow avoidance of unnecessary loads. In addition, there could be other useful future uses of a mechanism to properly load a resource as if it was being loaded by the page. So it would be nice to decouple this from the notification of discovering an icon link.

That's the full-on tangent!

As you know, "load an arbitrary subresource in the context of the page" has come up plenty of times over the history of the WK API, so it does seem like there's value here.
Offering that, however, does seem to make the "decode the icon image out of process" less of a fit with a general purpose API.

Yeah, we'd either have to provide a generic image decoding service or make a generic loading facility that's capable of vending decoded image data instead of raw data, which both seem a bit inelegant.

Asking these questions because if this is to be the One True Model of icon loading going forward, we should try to make sure the design is future-proof.

Yes, these are all great questions to ask.

Everything that's been brought up so far only really touches the API surface (Do we batch together icon load decision requests? Do we give an out-of-process decoded image instead of raw data? Do we offer an API for loading arbitrary subresource?)

Speaking with understanding of what's implemented today and what would be necessary to make any or all of these changes, I know that the current mechanism is a solid base on which these ideas can easily be built.

OK, as long as we still have room to address these issues before it becomes locked in as public API, then I am fine with not addressing these issues for now.

👍👍

Thanks,
~Brady


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Dan Bernstein


On Jun 16, 2017, at 11:25 AM, Brady Eidson <[hidden email]> wrote:


On Jun 16, 2017, at 11:11 AM, Maciej Stachowiak <[hidden email]> wrote:
This is slightly tangential, but a comment on the model: it doesn't seem like there's a way for clients to check what range of icons are available and only then choose which to load. Even though Safari may not have needed this to move over, if you wanted to do something rigorous like load the closest available size to what you need or else a scalable format, there's no way to do that without potentially loading icons you don't need. While Safari hasn't done this, it might be useful if Safari's various places that display icons are ever made more consistent and coherent.

This was discussed when implementing the model for Safari, which was admittedly done quickly.

While Safari didn't need what you suggest right now, they agreed they might need it in the future. Since the load decision request has a completion callback, and since it's a known implementation detail that they will all come in quick succession, it was accepted that the current model could work for "choosing the right icon of all choices" in a pinch.

How are you supposed to do this? Save all the completion handlers indefinitely? I can sort of see how that could work, but if the completion handler is intended to be used at a possibly later time than the shouldLoadIconWithParameters: method, it's kind of weird that it's a callback at all.

It definitely needs to be asynchronous because loading decisions can (often? always?) depend on i/o to a database.

Scenario:

1 - WebKit says “There’s a favicon of size 64x64 from URL “http://foo.com/favicon.ico” - Do you want me to load it?
2 - Client saves off that decision handler and queries its database to see if it wants that icon. (Do I have this one already? Do I have it but in a smaller size? Is my icon weeks old and therefore I want a refreshed one?)
3 - After the i/o for the database operation, the client decides it does (or does not) want this icon, and calls the completion handler.


I can see that there needs to be some per-icon notification, since <link> elements can be added after the fact, and also incremental parsing is a thing. So one notification that tells you all icons wouldn't cut it.

Right - The "just one icon" notification was the bare minimum because of this need.
We can definitely add a greater API surface to deliver a bulk "here's all the icons in the <head>" in addition to the individual icon notification.

Another minor comment: it seems like this new API returns raw data. It seems like the native way to use this would result in running untrusted data from the network through image decoders outside the Web Process sandbox. Do we have a way to avoid that?

This came up while implementing it for Safari, too. In practice we didn't decode icons out-of-process before so this model was not a regression. I see value in offering this, but it's also something conscientious clients can do on their own with the raw data.

It's not that easy for an arbitrary macOS app to make their own tightly sandboxed service for this, and probably impossible on iOS. So if we made this interface into public API, there wouldn't be a way for most clients to do the right thing.

There’s a *lot* of questions to answer before we even consider making this API.

Considering nothing related to the icon database has ever been API, there wasn’t a compelling reason to answer those questions this time around.

Are there things that the current SPI that Safari uses does that an app couldn’t do using user scripts? Naïvely, I’d expect a user script to be able to get and observe all meta and link elements in the DOM, to initiate resource loads in the context of the document, to be able to decode images by drawing into a canvas, and to deliver image data to the UI process using the message passing API.

Is the current SPI just serving as a convenience or efficiency measure so that Safari won’t have to do all of that in script (and/or in its injected bundle code)?


But separating the loading mechanism from the notification, plus adding a notification that the <head> section is done parsing, could allow avoidance of unnecessary loads. In addition, there could be other useful future uses of a mechanism to properly load a resource as if it was being loaded by the page. So it would be nice to decouple this from the notification of discovering an icon link.

That's the full-on tangent!

As you know, "load an arbitrary subresource in the context of the page" has come up plenty of times over the history of the WK API, so it does seem like there's value here.
Offering that, however, does seem to make the "decode the icon image out of process" less of a fit with a general purpose API.

Yeah, we'd either have to provide a generic image decoding service or make a generic loading facility that's capable of vending decoded image data instead of raw data, which both seem a bit inelegant.

Asking these questions because if this is to be the One True Model of icon loading going forward, we should try to make sure the design is future-proof.

Yes, these are all great questions to ask.

Everything that's been brought up so far only really touches the API surface (Do we batch together icon load decision requests? Do we give an out-of-process decoded image instead of raw data? Do we offer an API for loading arbitrary subresource?)

Speaking with understanding of what's implemented today and what would be necessary to make any or all of these changes, I know that the current mechanism is a solid base on which these ideas can easily be built.

OK, as long as we still have room to address these issues before it becomes locked in as public API, then I am fine with not addressing these issues for now.

👍👍

Thanks,
~Brady

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Geoffrey Garen
In reply to this post by Brady Eidson
>> Another minor comment: it seems like this new API returns raw data. It seems like the native way to use this would result in running untrusted data from the network through image decoders outside the Web Process sandbox. Do we have a way to avoid that?
>
> This came up while implementing it for Safari, too. In practice we didn't decode icons out-of-process before so this model was not a regression. I see value in offering this, but it's also something conscientious clients can do on their own with the raw data.

Didn’t we need to create the Safari ImageDecoder service to work around the problem of decoding untrusted icon images?

Geoff
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Dan Bernstein


On Jun 19, 2017, at 10:20 AM, Geoffrey Garen <[hidden email]> wrote:

>>> Another minor comment: it seems like this new API returns raw data. It seems like the native way to use this would result in running untrusted data from the network through image decoders outside the Web Process sandbox. Do we have a way to avoid that?
>>
>> This came up while implementing it for Safari, too. In practice we didn't decode icons out-of-process before so this model was not a regression. I see value in offering this, but it's also something conscientious clients can do on their own with the raw data.
>
> Didn’t we need to create the Safari ImageDecoder service to work around the problem of decoding untrusted icon images?

That’s not going to be available to other participants in the WebKit Open Source projects.

>
> Geoff
> _______________________________________________
> webkit-dev mailing list
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Geoffrey Garen
>>>> Another minor comment: it seems like this new API returns raw data. It seems like the native way to use this would result in running untrusted data from the network through image decoders outside the Web Process sandbox. Do we have a way to avoid that?
>>>
>>> This came up while implementing it for Safari, too. In practice we didn't decode icons out-of-process before so this model was not a regression. I see value in offering this, but it's also something conscientious clients can do on their own with the raw data.
>>
>> Didn’t we need to create the Safari ImageDecoder service to work around the problem of decoding untrusted icon images?
>
> That’s not going to be available to other participants in the WebKit Open Source projects.

Sorry — I don't mean to suggest that other projects should adopt Safari's ImageDecoder service. I just want to clarify that Maciej’s concern is more than theoretical.

I would add that I don’t like the idea that it’s the client’s job to be “conscientious” in order to achieve safe rendering of web content. The point of Modern WebKit as a framework is that all clients should get safe rendering by default.

Therefore, I think it’s a flaw that the current API vends only raw encoded data.

Geoff
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Carlos Garcia Campos-7
In reply to this post by Brady Eidson
El vie, 16-06-2017 a las 09:43 -0700, Brady Eidson escribió:
[...]

> > > If not, I can get rid of it right now
> > >
> > > If so, then I need a GTK maintainer to come up with a plan soon.
> >
> > How soon is soon? :-) We are approaching the end of our release
> > cycle,
> > it would be good for us if we could do any changes related to this
> > after we branch for 2.18. Of course I can branch earlier if needed.
> > According to our schedule we should branch the first week of
> > August. Is
> > that late?
>
> That’s much later than the soon I was targeting.
>
> Hoping to do this by the end of June.

Ok, I've found time today to work on this. I don't have time to re-
implement WebKitFaviconDatabase using a different database model, so
I've copied IconDatabase from WebCore to WebKit and used it directly
from WebKitFaviconDatabase. I've switched the client implementation to
API::IconLoadingClient too. So, I think you can just remove all the
WebCore and WebKit2 code after the patch lands.

https://bugs.webkit.org/show_bug.cgi?id=173877


Regards,
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Carlos Garcia Campos-7
El mar, 27-06-2017 a las 18:19 +0200, Carlos Garcia Campos escribió:

> El vie, 16-06-2017 a las 09:43 -0700, Brady Eidson escribió:
> [...]
> > > > If not, I can get rid of it right now
> > > >
> > > > If so, then I need a GTK maintainer to come up with a plan
> > > > soon.
> > >
> > > How soon is soon? :-) We are approaching the end of our release
> > > cycle,
> > > it would be good for us if we could do any changes related to
> > > this
> > > after we branch for 2.18. Of course I can branch earlier if
> > > needed.
> > > According to our schedule we should branch the first week of
> > > August. Is
> > > that late?
> >
> > That’s much later than the soon I was targeting.
> >
> > Hoping to do this by the end of June.
>
> Ok, I've found time today to work on this. I don't have time to re-
> implement WebKitFaviconDatabase using a different database model, so
> I've copied IconDatabase from WebCore to WebKit and used it directly
> from WebKitFaviconDatabase. I've switched the client implementation
> to
> API::IconLoadingClient too. So, I think you can just remove all the
> WebCore and WebKit2 code after the patch lands.
>
> https://bugs.webkit.org/show_bug.cgi?id=173877

I forgot to say that copying IconDatabase is just a temporary solution,
because I don't have time for a proper solution now, so during our next
release cycle I'll try to either improve or replace it with a new
implementation that allows to store multiple icons and sizes for the
same page, etc.

>
> Regards, 
> _______________________________________________
> webkit-dev mailing list
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|

Re: Intent to remove the WebCore::IconDatabase (GTK needs to make a decision)

Brady Eidson

> On Jun 27, 2017, at 9:36 AM, Carlos Garcia Campos <[hidden email]> wrote:
>
> El mar, 27-06-2017 a las 18:19 +0200, Carlos Garcia Campos escribió:
>> El vie, 16-06-2017 a las 09:43 -0700, Brady Eidson escribió:
>> [...]
>>>>> If not, I can get rid of it right now
>>>>>
>>>>> If so, then I need a GTK maintainer to come up with a plan
>>>>> soon.
>>>>
>>>> How soon is soon? :-) We are approaching the end of our release
>>>> cycle,
>>>> it would be good for us if we could do any changes related to
>>>> this
>>>> after we branch for 2.18. Of course I can branch earlier if
>>>> needed.
>>>> According to our schedule we should branch the first week of
>>>> August. Is
>>>> that late?
>>>
>>> That’s much later than the soon I was targeting.
>>>
>>> Hoping to do this by the end of June.
>>
>> Ok, I've found time today to work on this. I don't have time to re-
>> implement WebKitFaviconDatabase using a different database model, so
>> I've copied IconDatabase from WebCore to WebKit and used it directly
>> from WebKitFaviconDatabase. I've switched the client implementation
>> to
>> API::IconLoadingClient too. So, I think you can just remove all the
>> WebCore and WebKit2 code after the patch lands.
>>
>> https://bugs.webkit.org/show_bug.cgi?id=173877
>
> I forgot to say that copying IconDatabase is just a temporary solution,
> because I don't have time for a proper solution now, so during our next
> release cycle I'll try to either improve or replace it with a new
> implementation that allows to store multiple icons and sizes for the
> same page, etc.

This is great. Thanks!

I’ll review shortly.

~Brady

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev