Bug 123375 – WebIconDatabase can miss private browsing state changes

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

WebIconDatabase can miss private browsing state changes

[Brady Eidson]

In radar as <rdar://problem/15322318>

[Brady Eidson]

Created attachment 215230 [details]
Patch v1

[Brady Eidson]

http://trac.webkit.org/changeset/158075

[Alexey Proskuryakov]

Comment on attachment 215230 [details]
Patch v1 

View in context: https://bugs.webkit.org/attachment.cgi?id=215230&action=review

> Source/WebKit2/UIProcess/WebContext.cpp:432
> +    for (size_t i = 0, count = contexts.size(); i < count; ++i)
> +        contexts[i]->setPrivateBrowsingEnabled(true);

This is semantically incorrect. WebContext::willStopUsingPrivateBrowsing() is called when any context starts to use private browsing. Due to WebKit2 API limitations, the best thing we can do is create a private browsing session just in case. But it doesn't mean that all context start enter private browsing.

[Alexey Proskuryakov]

Also, this broke an API test on debug WK2 bot, so I'm going to roll out.

WebKit2.PrivateBrowsingPushStateNoHistoryCallback

[WebKit Commit Bot]

Re-opened since this is blocked by bug 123389

[Alexey Proskuryakov]

Comment on attachment 215230 [details]
Patch v1 

Rolled out in http://trac.webkit.org/changeset/158087

[Brady Eidson]

(In reply to comment #4)
> (From update of attachment 215230 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215230&action=review
> 
> > Source/WebKit2/UIProcess/WebContext.cpp:432
> > +    for (size_t i = 0, count = contexts.size(); i < count; ++i)
> > +        contexts[i]->setPrivateBrowsingEnabled(true);
> 
> This is semantically incorrect. WebContext::willStopUsingPrivateBrowsing() is called when any context starts to use private browsing. Due to WebKit2 API limitations, the best thing we can do is create a private browsing session just in case. But it doesn't mean that all context start enter private browsing.

Currently, any context being in private browsing means all contexts should be in private browsing.  

If I'm missing some way in which this is not true, please explain further.

I'm also not sure how the semantics are wrong, seeing as WebContexts own their IconDatabases.

I agree willStopUsingPrivateBrowsing was added for private browsing session management, but it's name does not at all suggest it should only be limited to that use.

[Brady Eidson]

(In reply to comment #5)
> Also, this broke an API test on debug WK2 bot, so I'm going to roll out.
> 
> WebKit2.PrivateBrowsingPushStateNoHistoryCallback

Without even including details of what broke?

[Brady Eidson]

The failure:

ASSERTION FAILED: m_ptr
/Volumes/Data/CabUser/build/Debug/usr/local/include/wtf/OwnPtr.h(56) : PtrType WTF::OwnPtr<WebCore::IconDatabase>::operator->() const [T = WebCore::IconDatabase]
1   0x10ee0f5a0 WTFCrash
2   0x110620825 WTF::OwnPtr<WebCore::IconDatabase>::operator->() const
3   0x110620514 WebKit::WebIconDatabase::setPrivateBrowsingEnabled(bool)
4   0x110583e2f WebKit::WebContext::setPrivateBrowsingEnabled(bool)
5   0x110583dce WebKit::WebContext::willStartUsingPrivateBrowsing()
6   0x11079896a WebKit::WebPreferences::addPageGroup(WebKit::WebPageGroup*)
7   0x1106a6ef3 WebKit::WebPageGroup::setPreferences(WebKit::WebPreferences*)
8   0x110834542 WKPageGroupSetPreferences
...

Got it.  Will fix in a follow up.

[Brady Eidson]

(In reply to comment #10)
> The failure:
> 
> ASSERTION FAILED: m_ptr
> /Volumes/Data/CabUser/build/Debug/usr/local/include/wtf/OwnPtr.h(56) : PtrType WTF::OwnPtr<WebCore::IconDatabase>::operator->() const [T = WebCore::IconDatabase]
> 1   0x10ee0f5a0 WTFCrash
> 2   0x110620825 WTF::OwnPtr<WebCore::IconDatabase>::operator->() const
> 3   0x110620514 WebKit::WebIconDatabase::setPrivateBrowsingEnabled(bool)
> 4   0x110583e2f WebKit::WebContext::setPrivateBrowsingEnabled(bool)
> 5   0x110583dce WebKit::WebContext::willStartUsingPrivateBrowsing()
> 6   0x11079896a WebKit::WebPreferences::addPageGroup(WebKit::WebPageGroup*)
> 7   0x1106a6ef3 WebKit::WebPageGroup::setPreferences(WebKit::WebPreferences*)
> 8   0x110834542 WKPageGroupSetPreferences
> ...
> 
> Got it.  Will fix in a follow up.

Actually, this is odd... having a null icon database seems impossible for a WebContext that hasn't already been torn down...  yikes.

[Brady Eidson]

(In reply to comment #11)
> (In reply to comment #10)
> > Got it.  Will fix in a follow up.
> 
> Actually, this is odd... having a null icon database seems impossible for a WebContext that hasn't already been torn down...  yikes.

NM, got it.

[Brady Eidson]

Created attachment 215264 [details]
Patch v2 - Add a necessary null check, more comprehensively set the private browsing value at icon database creation time.

This patch fixes the crash, and is better at the time of icon database creation.

I'm not sure why Alexey states that a WebContext notification for starting/stopping private browsing isn't a good place to do private browsing maintenance for WebContext-owned objects, but hopefully he can explain along with a review!  :)

[Alexey Proskuryakov]

> Currently, any context being in private browsing means all contexts should be in private browsing.  

I don't think that this is accurate, Safari certainly doesn't guarantee that.

> > WebKit2.PrivateBrowsingPushStateNoHistoryCallback

> Without even including details of what broke?

What details do you think were missing? API tests don't report anything but the name of the failing test.

[Brady Eidson]

(In reply to comment #14)
> > > WebKit2.PrivateBrowsingPushStateNoHistoryCallback
> 
> > Without even including details of what broke?
> 
> What details do you think were missing? API tests don't report anything but the name of the failing test.

Well it was an ASSERT with a backtrace...  :)  It's fixed in this patch.

> > Currently, any context being in private browsing means all contexts should be in private browsing.  
> 
> I don't think that this is accurate, Safari certainly doesn't guarantee that.
> 

Not talking about Safari, talking about WebKit - Currently, even though there have been various efforts to make private browsing more fine-grained, it really is a single global setting.  Is there any place where changing the WKPreference does not filter down in to every Context, every PageGroup, every Page?

But all of this discussion aside, in WK2, the icon database is a per-WebContext concept. Therefore it makes sense to change the private browsing flag when the WebContext gets notified of a change in the flag.

What I see in the code is are methods named "willStartUsingPrivateBrowsing" and "willStopUsingPrivateBrowsing".  They implicitly concede that private browsing is currently a global setting, in that they are static methods that notify all instances.  Additionally, their names don't suggest they have anything "special" about them that makes them insufficient for doing any type of private browsing related work I see fit.

If this patch is really wrong, can I have an alternate suggestion?

[Alexey Proskuryakov]

> Well it was an ASSERT with a backtrace...  :)

Ah indeed, there was a stack trace in the log. Sorry, I just didn't see it there, I only noticed the name in the summary at the end. Anyway, glad that it was easily reproducible for you.

> Not talking about Safari, talking about WebKit - Currently, even though there have been various efforts to make private browsing more fine-grained, it really is a single global setting.  Is there any place where changing the WKPreference does not filter down in to every Context, every PageGroup, every Page?

Setting the preference doesn't filter down anywhere beyond what it was explicitly set on. That is, the page groups associated with this particular instance of WKPreferences.

WK_EXPORT void WKPreferencesSetPrivateBrowsingEnabled(WKPreferencesRef preferencesRef, bool enabled);

As explained in comment 4, the static willStart/willStopUsingPrivateBrowsing functions in WebContext exist so that we could create a private browsing session in each WebProcess and in NetworkProcess, and - which is more tricky - to delete the sessions when exiting private browsing. But whether network requests will use this session is decided by Settings associated with each Page, not by whether the session exists in the process.

> But all of this discussion aside, in WK2, the icon database is a per-WebContext concept.

IIRC, a PageGroup can span multiple WebContexts, and of course each WebContext can host multiple PageGroups. Anything that is per-WebContext and depends on preferences such as private browsing is  broken by design.

I'd be surprised if there was a way to fix it in WebKit for arbitrary legitimate uses of the existing API. I still think that the patch may be correct for Safari's use, and all you need is to rename the WebContext::setPrivateBrowsingEnabled function added here to something more like WebContext::setSomePageGroupsMightHavePrivateBrowsingEnabled.

I also think that the time we already spent trying to hack around the existing private browsing API is more than it would take to make a new clean one.

[Brady Eidson]

(In reply to comment #16)
> > But all of this discussion aside, in WK2, the icon database is a per-WebContext concept.
> 
> IIRC, a PageGroup can span multiple WebContexts, and of course each WebContext can host multiple PageGroups. Anything that is per-WebContext and depends on preferences such as private browsing is broken by design.

A lot of things that depend on Preferences/Settings are broken by design  :(

> I'd be surprised if there was a way to fix it in WebKit for arbitrary legitimate uses of the existing API. I still think that the patch may be correct for Safari's use, and all you need is to rename the WebContext::setPrivateBrowsingEnabled function added here to something more like WebContext::setSomePageGroupsMightHavePrivateBrowsingEnabled.

I will do so!

> I also think that the time we already spent trying to hack around the existing private browsing API is more than it would take to make a new clean one.

Can't wait for us to get to that, but doing so is also way out of the scope of fixing this.

[Brady Eidson]

Created attachment 215275 [details]
Patch v3

[Brady Eidson]

http://trac.webkit.org/changeset/158101