Bug 68208 – Sites that use history pushState or replaceState are recorded in history in Private Browsing mode

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jessie Berlin]

Neither HistoryController::pushState nor HistoryController::replaceState check if private browsing is enabled before adding visited links and calling FrameLoaderClient::updateGlobalHistory.

<rdar://problem/9945573>

[Jessie Berlin]

Created attachment 107585 [details]
Patch

[Andy Estes]

Can a test not be written for this?

[Brady Eidson]

I'm not convinced this is actually a WebKit bug.  Shouldn't this be a client app policy?

[Jessie Berlin]

(In reply to comment #2)
> Can a test not be written for this?

I was trying to come up with ways to test this. I thought about a TestWebKitAPI test, but realized that I would be trying to assert that the WKContextDidNavigateWithNavigationDataCallback did not happen when Private Browsing is enabled - but there would be no way to determine when it was done “not happening”.

Do you have any suggestions?

[Jessie Berlin]

(In reply to comment #3)
> I'm not convinced this is actually a WebKit bug.  Shouldn't this be a client app policy?

This checking for privateBrowsingEnabled before calling addVisitedLink and updateGlobalHistory is done throughout HistoryController. I don’t see why these should be the exception.

[Brady Eidson]

(In reply to comment #5)
> (In reply to comment #3)
> > I'm not convinced this is actually a WebKit bug.  Shouldn't this be a client app policy?
> 
> This checking for privateBrowsingEnabled before calling addVisitedLink and updateGlobalHistory is done throughout HistoryController. I don’t see why these should be the exception.

I guess it shouldn't be.  But I think those were a bad call.  They're leftover from when WebKit did history itself.  oh well.

[Andy Estes]

(In reply to comment #4)
> Do you have any suggestions?

You could write a layout test that asserts that links aren't considered visited after a pushState with private browsing enabled. I believe there is a special LTC method to get the visited state of a link.

[Jessie Berlin]

Comment on attachment 107585 [details]
Patch

I am going to try out Andy's suggestion for a test.

[Jessie Berlin]

Created attachment 107759 [details]
Patch (take 2 - with API test)

[WebKit Review Bot]

Comment on attachment 107759 [details]
Patch (take 2 - with API test)

Attachment 107759 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9725662

New failing tests:
svg/W3C-SVG-1.1/animate-elem-12-t.svg
svg/W3C-SVG-1.1/animate-elem-04-t.svg
svg/W3C-SVG-1.1/animate-elem-07-t.svg
platform/chromium/fast/text/text-stroke-with-border.html
svg/W3C-SVG-1.1/animate-elem-08-t.svg
svg/W3C-SVG-1.1/animate-elem-09-t.svg
printing/return-from-printing-mode.html
platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html
svg/W3C-SVG-1.1/animate-elem-11-t.svg
svg/W3C-SVG-1.1/animate-elem-05-t.svg
svg/W3C-SVG-1.1/animate-elem-02-t.svg
svg/W3C-SVG-1.1/animate-elem-03-t.svg
svg/W3C-SVG-1.1/animate-elem-06-t.svg
svg/W3C-SVG-1.1/animate-elem-13-t.svg
svg/W3C-SVG-1.1/animate-elem-10-t.svg

[Jessie Berlin]

(In reply to comment #10)
> (From update of attachment 107759 [details])
> Attachment 107759 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9725662
> 
> New failing tests:
> svg/W3C-SVG-1.1/animate-elem-12-t.svg
> svg/W3C-SVG-1.1/animate-elem-04-t.svg
> svg/W3C-SVG-1.1/animate-elem-07-t.svg
> platform/chromium/fast/text/text-stroke-with-border.html
> svg/W3C-SVG-1.1/animate-elem-08-t.svg
> svg/W3C-SVG-1.1/animate-elem-09-t.svg
> printing/return-from-printing-mode.html
> platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html
> svg/W3C-SVG-1.1/animate-elem-11-t.svg
> svg/W3C-SVG-1.1/animate-elem-05-t.svg
> svg/W3C-SVG-1.1/animate-elem-02-t.svg
> svg/W3C-SVG-1.1/animate-elem-03-t.svg
> svg/W3C-SVG-1.1/animate-elem-06-t.svg
> svg/W3C-SVG-1.1/animate-elem-13-t.svg
> svg/W3C-SVG-1.1/animate-elem-10-t.svg

Not sure why these tests would be failing after my change - they don’t appear to deal with private browsing or visited links. Notably, they also didn’t fail on the chromium-mac ews ...

[Jessie Berlin]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 107759 [details] [details])
> > Attachment 107759 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/9725662
> > 
> > New failing tests:
> > svg/W3C-SVG-1.1/animate-elem-12-t.svg
> > svg/W3C-SVG-1.1/animate-elem-04-t.svg
> > svg/W3C-SVG-1.1/animate-elem-07-t.svg
> > platform/chromium/fast/text/text-stroke-with-border.html
> > svg/W3C-SVG-1.1/animate-elem-08-t.svg
> > svg/W3C-SVG-1.1/animate-elem-09-t.svg
> > printing/return-from-printing-mode.html
> > platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html
> > svg/W3C-SVG-1.1/animate-elem-11-t.svg
> > svg/W3C-SVG-1.1/animate-elem-05-t.svg
> > svg/W3C-SVG-1.1/animate-elem-02-t.svg
> > svg/W3C-SVG-1.1/animate-elem-03-t.svg
> > svg/W3C-SVG-1.1/animate-elem-06-t.svg
> > svg/W3C-SVG-1.1/animate-elem-13-t.svg
> > svg/W3C-SVG-1.1/animate-elem-10-t.svg
> 
> Not sure why these tests would be failing after my change - they don’t appear to deal with private browsing or visited links. Notably, they also didn’t fail on the chromium-mac ews ...

AND they didn’t fail with the first version of this patch. The only difference between the first and second version is that the second version has a test.

[Jessie Berlin]

Comment on attachment 107759 [details]
Patch (take 2 - with API test)

Thanks Brady! Committed in http://trac.webkit.org/changeset/95440