Bug 24300

Created attachment 28196 [details] CSSKeepVisitedLinksPrivate patch Tested with Arora.

Comment on attachment 28196 [details] CSSKeepVisitedLinksPrivate patch This also disables display of :visited styles, correct? If so, this needs a better setting name. Like visitedPseudoStyleEnabled().

Created attachment 31664 [details] Updated patch Still unsure of what to call the option but have altered it to the more explicit 'CSSIgnoreVisitedLinks'. Revisions include: - addition of a manual test-case - the original patch wasn't working properly - this one has been thoroughly tested - um, that's it.

Comment on attachment 31664 [details] Updated patch > + Settings* settings = m_document->settings(); > + if (!settings || settings->cssIgnoreVisitedLinks()) > + m_IgnoreVisitedLinks = true; This should default to false if we can't find settings. > +++ b/WebCore/manual-tests/css-ignore-visited-links.html This should be a LayoutTest instead of a manual test. See something like xssAuditor to see how to test non-default settings in LayoutTests.

Created attachment 31746 [details] Updated patch with layout test m_IgnoreVisitedLinks is already set to false by default by the constructor. won't that do the trick? i've added a layout test - had to add a stub history manager to the qt dumprendertree to support it.

Comment on attachment 31746 [details] Updated patch with layout test This is looking good! Below are some minor style issues. Once we address those, we're all set. > +function runTest() { > + if (window.layoutTestController) { > + layoutTestController.notifyDone(); > + } > +} No braces around one-line if statements. > + Settings* settings = m_document->settings(); > + if (!settings || settings->cssIgnoreVisitedLinks()) > + m_IgnoreVisitedLinks = true; If |settings| is null, then m_IgnoreVisitedLinks gets set to true, which is isn't what we want (as I said in my previous review). > @@ -262,7 +265,6 @@ QString DumpRenderTree::dumpFramesAsText(QWebFrame* frame) > > QString DumpRenderTree::dumpBackForwardList() > { > - QString result; > result.append(QLatin1String("\n============== Back Forward List ==============\n")); > result.append(QLatin1String("FIXME: Unimplemented!\n")); > result.append(QLatin1String("===============================================\n")); I don't understand why you removed this line. Is this just unrelated cleanup? > QWebPage *m_page; > + HistoryManager *historyManager; These * are misplaced. I see that you're matching the style of this file, however. > +HistoryManager::HistoryManager(QObject *parent) > + : QWebHistoryInterface(parent) > + ,m_count(0) Missing a space after the , here. > + m_count++; Prefer pre-increment (e.g, ++m_count). Will this test fail on other platforms since you only implemented this API for QT? Maybe you should update the skipped test lists for those platforms to avoid turning the tree red?

Created attachment 31753 [details] Updated patch with suggested changes oops - couple of clangers in there. thanks! have ignored 2 items - retained the style in dumprendertree.h (because the rest of the file is like that as you note) and the unit-tests are already in qt's platform specific folder.

Comment on attachment 31753 [details] Updated patch with suggested changes Awesome! Thanks for fixing the nits.

I don't think the license in patch in (for historymanager.cpp) is one of the ones we accept for patches in WebKit. I would suggest not landing this change until it has been worked out.

Comment on attachment 31753 [details] Updated patch with suggested changes This patch is not BSD or LGPL. Rolling out. It should never have been r+'d.

Oh, nm. I never landed it. :) So not rolling it out... but r-ing it. :)

Comment on attachment 31753 [details] Updated patch with suggested changes This patch includes GPL-licensed code, so it can't be licensed as-is. WebKit code has to be LGPL or BSD-style licensed. Changing to r-.

Created attachment 31797 [details] Patch with Update Copyright Notices I've updated the copyright notice on historymanger.* and also tweaked a few bits. Just FYI I started out with http://github.com/Arora/arora/tree/master/src/history/historymanager.cpp and ended up removing everything except the minimum required to support the tests. The contents of the remaining functions are entirely different from the original and their names are defined by qwebhistoryinterface.

Comment on attachment 31797 [details] Patch with Update Copyright Notices (In reply to comment #13) > Just FYI I started out with > http://github.com/Arora/arora/tree/master/src/history/historymanager.cpp INAL, but it sounds like this patch is a derived work from the above URI and therefore subject to the GPL. Are you sure you have the legal right to relicense the code? (I'm pretty sure that's what you warrant when you click through the patch upload agreement.)

(In reply to comment #14) > (From update of attachment 31797 [details] [review]) > (In reply to comment #13) > > Just FYI I started out with > > http://github.com/Arora/arora/tree/master/src/history/historymanager.cpp > INAL, but it sounds like this patch is a derived work from the above URI and > therefore subject to the GPL. Are you sure you have the legal right to > relicense the code? (I'm pretty sure that's what you warrant when you click > through the patch upload agreement.) I believe I have ended up with a completely different class - in fact it is a minimal class required to support qwebhistoryinterface. I understand that the way I wrote the class has created ambiguity, so you are right to be cautious. At this point the only common elements between my historymanager.cpp and the one in Arora are: - the filenames - the name of the class (HistoryManager) The only function names it shares with arora's are the ones that it is required to reimplement from qwebhistoryinterface in QtWebKit in order to function - addHistoryEntry and historyContains. The contents of both these functions is different from the arora file. Can you advise what I need to do dispel the whiff of sulphur here?

> Can you advise what I need to do dispel the whiff of sulphur here? That question is beyond my pay grade... Hopefully someone with more knowledge of these issues will chime in.

I reviewed the patch and did not find it to be a derivative of (my) Arora's history manager. Any subclass of the history interface will look similar to both this and Arora's.

Created attachment 31865 [details] New class DRTHistory replacing HistoryManager If it helps, I've updated the patch with two completely new files drthistory.cpp and drthistory.h and removed historymanager.cpp and historymanager.. The class they implement is now called DRTHistory. I'll also submit a diff between the arora historymanager.cpp/h and drthistory.cpp/h. Hope this isn't OTT but just documenting that they are demonstrably non-derivative.

Created attachment 31867 [details] diff as mentioned in previous comment

Comment on attachment 31865 [details] New class DRTHistory replacing HistoryManager Don't you want to return PseudoAnyLink instead of PseudoLink? If so, it seems the tests need to be updated to catch that case. Otherwise this looks fine.

Comment on attachment 31865 [details] New class DRTHistory replacing HistoryManager if (!m_ignoreVisitedLinks && pseudoState == PseudoVisited) Why is the above change necessary? If you hack checkPseudoState to always pretend like you aren't visited, then why do you need this extra code?

(In reply to comment #21) > (From update of attachment 31865 [details] [review]) > if (!m_ignoreVisitedLinks && pseudoState == PseudoVisited) > > Why is the above change necessary? If you hack checkPseudoState to always > pretend like you aren't visited, then why do you need this extra code? > You're right - this line is redundant. I've tested it without it, and manual and auto tests still pass. I think it was introduced in an earlier stage in the patch, when for some reason I had the check in checkPseudoState in the wrong place.

Created attachment 32098 [details] Updated patch Updated to remove redundant check.

Comment on attachment 32098 [details] Updated patch I thought the QWebHistoryInterface interface was deprecated? I know that WebKit qt has some hacks in place around the WebCore link hashing system just to support this interface. Seeems we should not be using it from DRT if something better is available. Style: 92 DRTHistory *historyManager; Leaked: 154 historyManager = new DRTHistory(m_page); You tests shoudl ideally e dumpAsText, since all they need to do is check the computedStyle, they don't need to dump the pixels for the page to confirm this change.

AFAIK, we didn't deprecate QWebHistoryInterface as we have no other history API in place. If we should do so, now would be the time before our Qt 4.6 release. CC'ing Simon to see if he has some input.

Created attachment 34405 [details] Updated patch Thanks Eric - patch updated per your comments. Hopefully this patch will do if it's decided that moving from the deprecated QWebHistoryInterface API is another day's work.

It seems from this commit that ripping this bad API out of Qt was slated for 4.5... I guess that never happened? http://trac.webkit.org/changeset/43052

(In reply to comment #27) > It seems from this commit that ripping this bad API out of Qt was slated for > 4.5... I guess that never happened? It will not happen. The API needs to stay like this until Qt5.0. What Nokia can and should do (note I'm not with Nokia) is to create a new API that follows the "push" history into WebCore model and deprecate the old one.

FYI: a similar bug entry for Mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=147777 They have discussed this issue for 7 years.

Comment on attachment 34405 [details] Updated patch Does qt have support yet for the layoutTestController.overridePreference stuff? If so, this test really should use that and not be qt-specific (skipped by all the other platforms if they don't want to add this preference). I think this patch is OK. Just not very excited about it. Especially with it using qt API which we were hoping to see die some day. ;)

> (From update of attachment 34405 [details]) > Does qt have support yet for the layoutTestController.overridePreference stuff? > If so, this test really should use that and not be qt-specific (skipped by all > the other platforms if they don't want to add this preference). Unfortunately qt does not support overridePreference at the moment. > > I think this patch is OK. Just not very excited about it. Especially with it > using qt API which we were hoping to see die some day. ;) In the patch's defence, it is adding support for the current qt history API to qt's DRT in order to permit an autotest. The feature itself doesn't depend on this API. Does that make a difference?

I admit I feel a certain hesitation of adding a feature to the QtWebKit API that isn't used by any other port (despite the fact that it appears useful). So if all this is solved on a different level in the future, then this setting in the API would become obsolete I guess?

(In reply to comment #32) > I admit I feel a certain hesitation of adding a feature to the QtWebKit API > that isn't used by any other port (despite the fact that it appears useful). I think there are people from other ports cc'd on the bug so they can chime in if they intend to use the setting. Perhaps there is usually some consensus built with other parts before adding a setting to webcore? It's there if they want to use it I guess. It might be a natural fit with the 'Private Browsing' mode offered by some browsers. > > So if all this is solved on a different level in the future, then this setting > in the API would become obsolete I guess? Yes, it would. Solving it without a setting - i.e. allowing the CSS style to display normally without leaking the information remotely - appears to be an intractable problem at this point. But if a solution is ever found, then it would make this setting obsolete.

Comment on attachment 34405 [details] Updated patch This patch seems fine, but please add Preference APIs for the other platforms so we can have better test coverage of this feature. Also m_ignoreVisitedLinks isn't the greatest name. We're not ignoring visited links. We're ignoring the history for the purpose of computing the :visited pseud-selector.

(In reply to comment #34) > (From update of attachment 34405 [details]) > This patch seems fine, but please add Preference APIs for the other platforms > so we can have better test coverage of this feature. I will be able to add and test it for gtk and possibly win and wx, but I don't have the build environment for mac, so let me know if that's not enough. > Also m_ignoreVisitedLinks > isn't the greatest name. We're not ignoring visited links. We're ignoring the > history for the purpose of computing the :visited pseud-selector. OK. m_ignoreHistoryForCSS ? (I'm bad at names as you can tell)

Created attachment 44334 [details] Update Patch Updated patch adding preference and unit tests to gtk, qt, and win ports.

For the Qt port I would prefer a positive naming of the attribute in QWebSettings, i.e. FooEnabled with a default to true - simply for consistency. What do the other Qt hackers think? :)

Created attachment 44360 [details] updated patch Updated per Simon's comments - option is now CSSVisitedStyleEnabled and set to true by default for all platforms.

Attachment 44360 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebsettings.cpp:101: css_visited_style_enabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:2548: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 2

Created attachment 44364 [details] Update to skip test for Mac

Comment on attachment 44364 [details] Update to skip test for Mac > @@ -873,7 +873,11 @@ CSSStyleSelector::SelectorChecker::SelectorChecker(Document* document, bool stri > , m_collectRulesOnly(false) > , m_pseudoStyle(NOPSEUDO) > , m_documentIsHTML(document->isHTMLDocument()) > + , m_cssVisitedStyleEnabled(true) In a class named CSSStyleSelector, it seems that include "css" in the name of the data member is not helpful. The best boolean data member names are things that complete a phrase "style selector <xxx>" so I would call this m_ignoresVisitedLinks. I'm also not sure the CSS style selector is the right level for this. The PageGroup class already has a function setShouldTrackVisitedLinks. Do we really need to add something else? I'm going to say review- for now so you can consider both of those comments.

(In reply to comment #41) > (From update of attachment 44364 [details]) > > @@ -873,7 +873,11 @@ CSSStyleSelector::SelectorChecker::SelectorChecker(Document* document, bool stri > > , m_collectRulesOnly(false) > > , m_pseudoStyle(NOPSEUDO) > > , m_documentIsHTML(document->isHTMLDocument()) > > + , m_cssVisitedStyleEnabled(true) > > In a class named CSSStyleSelector, it seems that include "css" in the name of > the data member is not helpful. The best boolean data member names are things > that complete a phrase "style selector <xxx>" so I would call this > m_ignoresVisitedLinks. > From comment #34: > Also m_ignoreVisitedLinks > isn't the greatest name. We're not ignoring visited links. We're ignoring the > history for the purpose of computing the :visited pseud-selector. :-) I think you read that comment and are disagreeing with Adam - so I'll change it back again if no one still objects.. or let me know. > I'm also not sure the CSS style selector is the right level for this. The > PageGroup class already has a function setShouldTrackVisitedLinks. Do we really > need to add something else? > I should have mentioned in the original bug report that I decided against this because the purpose of the setting is to conceal visited links from CSS exploits rather than not track visited links at all (and wipe from the hash db any already visited). This seemed the less intrusive way of implementing it, since the user may find it confusing when they switch *out* of the option and find that links which were displayed as visited before they switched are now displayed as not-visited. > I'm going to say review- for now so you can consider both of those comments.

Comment on attachment 44364 [details] Update to skip test for Mac See comment #42

Comment on attachment 44364 [details] Update to skip test for Mac The EWS builders seem to think this patch does not apply. Meaning it won't be able to be cq'd even if r+'d.

http://webkit-commit-queue.appspot.com/patch/44364 is the EWS status link btw.

Comment on attachment 44364 [details] Update to skip test for Mac This patch does not apply and appears abandoned. r-

Created attachment 48915 [details] Updated Patch God loves a trier!

This new patch also does not apply, as you can tell by the purple bubbles below the patch. They should be green (except for Mac) if your patch applies correctly and builds.

Created attachment 49155 [details] Updated Patch

Attachment 49155 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebsettings.cpp:1200: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Ignoring "WebKit/qt/Api/qwebsettings.cpp": this file is exempt from the style guide. Ignoring "WebKit/qt/Api/qwebsettings.h": this file is exempt from the style guide. Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 49157 [details] Updated Patch

Comment on attachment 49157 [details] Updated Patch I haven't discussed this with other members of the project, but I think we should fix this by default instead of adding an option. One approach that can work by default is the one outlined here: http://dbaron.org/mozilla/visited-privacy

I'm going to implement dbaron's solution to this problem. It's great. That way we don't need an extra option for this.

Taking the bug.

Another consideration on top of what dbaron did is that if we ever allow for the drawing of HTML elements into a canvas, we will need to have a notion that the paint is "untrusted", and only paint the unvisited styles. For now I'll punt on that, but I'll put a FIXME in the drawing function that will mention this future issue.

Created attachment 52338 [details] Work in progress.

Created attachment 52436 [details] This patch gets all the drawing code changed properly (except for SVG fill and stroke). This patch gets all the drawing code for border, outlines, column rules, etc. patched properly. SVG fill and stroke still need to be patched. visitedDependentColor right now still just returns the unvisited style color, so the next step is to patch it to really fetch the visited style. It will do this for both visited and unvisited links to prevent timing attacks based off slow style computation.

Created attachment 52451 [details] Fix collapsed border drawing to propagate colors from the original elements participating in the collapse To get collapsed borders in tables working, I had to fix bugs with color propagation in those functions. Now a CollapsedBorderValue will make sure to bring along the color property from the original element if no border color is explicitly set. The current code in TOT always uses the cell's current color when no border color is set on the winning border style, and that's wrong.

dhyatt and I chatted about timing attacks on #webkit the other day. dbaron has a timing testing that you might be interested in testing your patch against: https://bugzilla.mozilla.org/show_bug.cgi?id=147777#c35

Created attachment 52552 [details] This patch gets the feature working!

Created attachment 52572 [details] Add SVG fill/stroke support

Some notes: My biggest worry is performance (and to a lesser extent footprint). All links and their descendants effectively have two styles. The time to compute the style information for these objects has doubled, and the style footprint for these objects has doubled. I'm not actually computing an unvisited and a visited style for each node. I'm computing an unvisited and real style for each node. If the real style is also unvisited then you end up with duplicated styles. It might be better to compute a forced visited style for each node, so that the two styles are actually unvisited and visited. Doing so means I'd need to know if the link is actually visited at paint time rather than just always using the visited color. I pretty much have to do this to avoid timing attacks though.

Created attachment 52582 [details] Ready for a first review pass This patch is ready for someone to take a look at it. It's kind of an open question how we want to do tests. This feature is extremely difficult to test using our layout test system, so I need to think about what the best way to test it is.

Isn't this trivial to test with pixel tests?

Attachment 52582 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/rendering/style/RenderStyle.cpp:502: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/rendering/style/RenderStyle.h:178: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/rendering/style/RenderStyle.h:204: _insideLink is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/rendering/style/RenderStyle.h:249: _styleType is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/rendering/style/RenderStyle.h:255: _isLink is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/rendering/RenderObject.cpp:828: Missing spaces around / [whitespace/operators] [3] WebCore/rendering/RenderObject.cpp:834: Missing spaces around / [whitespace/operators] [3] WebCore/rendering/RenderObject.cpp:2302: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/rendering/RenderObject.cpp:2303: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/rendering/InlineFlowBox.cpp:24: Alphabetical sorting problem. [build/include_order] [4] WebCore/platform/LinkHash.cpp:194: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 11 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 52582 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/1651197

<rdar://problem/6407151>

Created attachment 52738 [details] Patch that fixes link hashing for <a href="">

Comment on attachment 52738 [details] Patch that fixes link hashing for <a href=""> The code change looks good. But no regression test makes me sad!

Created attachment 52778 [details] Patch including ChangeLogs and tests

Comment on attachment 52778 [details] Patch including ChangeLogs and tests r=me, but you need to fix the issues i mentioned on irc: correct bit count, windows drt stuff in particular

Fixed in r57292.