Bug 68208 - Sites that use history pushState or replaceState are recorded in history in Private Browsing mode
Summary: Sites that use history pushState or replaceState are recorded in history in P...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jessie Berlin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-09-15 19:10 PDT by Jessie Berlin
Modified: 2011-09-19 11:00 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.98 KB, patch)
2011-09-15 19:21 PDT, Jessie Berlin
no flags Details | Formatted Diff | Diff
Patch (take 2 - with API test) (12.86 KB, patch)
2011-09-16 18:11 PDT, Jessie Berlin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Berlin 2011-09-15 19:10:18 PDT
Neither HistoryController::pushState nor HistoryController::replaceState check if private browsing is enabled before adding visited links and calling FrameLoaderClient::updateGlobalHistory.

<rdar://problem/9945573>
Comment 1 Jessie Berlin 2011-09-15 19:21:36 PDT
Created attachment 107585 [details]
Patch
Comment 2 Andy Estes 2011-09-15 20:47:19 PDT
Can a test not be written for this?
Comment 3 Brady Eidson 2011-09-15 21:34:41 PDT
I'm not convinced this is actually a WebKit bug.  Shouldn't this be a client app policy?
Comment 4 Jessie Berlin 2011-09-16 07:52:36 PDT
(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?
Comment 5 Jessie Berlin 2011-09-16 07:53:47 PDT
(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.
Comment 6 Brady Eidson 2011-09-16 08:16:11 PDT
(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.
Comment 7 Andy Estes 2011-09-16 08:51:35 PDT
(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.
Comment 8 Jessie Berlin 2011-09-16 09:16:16 PDT
Comment on attachment 107585 [details]
Patch

I am going to try out Andy's suggestion for a test.
Comment 9 Jessie Berlin 2011-09-16 18:11:36 PDT
Created attachment 107759 [details]
Patch (take 2 - with API test)
Comment 10 WebKit Review Bot 2011-09-17 16:53:30 PDT
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
Comment 11 Jessie Berlin 2011-09-19 10:02:43 PDT
(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 ...
Comment 12 Jessie Berlin 2011-09-19 10:03:54 PDT
(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.
Comment 13 Jessie Berlin 2011-09-19 11:00:16 PDT
Comment on attachment 107759 [details]
Patch (take 2 - with API test)

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