RESOLVED FIXED 68208
Sites that use history pushState or replaceState are recorded in history in Private Browsing mode
https://bugs.webkit.org/show_bug.cgi?id=68208
Summary Sites that use history pushState or replaceState are recorded in history in P...
Jessie Berlin
Reported 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>
Attachments
Patch (1.98 KB, patch)
2011-09-15 19:21 PDT, Jessie Berlin
no flags
Patch (take 2 - with API test) (12.86 KB, patch)
2011-09-16 18:11 PDT, Jessie Berlin
no flags
Jessie Berlin
Comment 1 2011-09-15 19:21:36 PDT
Andy Estes
Comment 2 2011-09-15 20:47:19 PDT
Can a test not be written for this?
Brady Eidson
Comment 3 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?
Jessie Berlin
Comment 4 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?
Jessie Berlin
Comment 5 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.
Brady Eidson
Comment 6 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.
Andy Estes
Comment 7 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.
Jessie Berlin
Comment 8 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.
Jessie Berlin
Comment 9 2011-09-16 18:11:36 PDT
Created attachment 107759 [details] Patch (take 2 - with API test)
WebKit Review Bot
Comment 10 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
Jessie Berlin
Comment 11 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 ...
Jessie Berlin
Comment 12 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.
Jessie Berlin
Comment 13 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
Note You need to log in before you can comment on or make changes to this bug.