Bug 24300 - Keep visited links private so that history info isn't leaked.
Summary: Keep visited links private so that history info isn't leaked.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Enhancement
Assignee: Dave Hyatt
URL:
Keywords: InRadar, Qt
Depends on:
Blocks: 37902
  Show dependency treegraph
 
Reported: 2009-03-02 13:46 PST by Robert Hogan
Modified: 2010-05-07 08:00 PDT (History)
16 users (show)

See Also:


Attachments
CSSKeepVisitedLinksPrivate patch (14.42 KB, patch)
2009-03-02 13:48 PST, Robert Hogan
eric: review-
Details | Formatted Diff | Diff
Updated patch (12.76 KB, patch)
2009-06-22 12:14 PDT, Robert Hogan
abarth: review-
Details | Formatted Diff | Diff
Updated patch with layout test (27.80 KB, patch)
2009-06-23 15:29 PDT, Robert Hogan
abarth: review-
Details | Formatted Diff | Diff
Updated patch with suggested changes (27.41 KB, patch)
2009-06-23 15:59 PDT, Robert Hogan
eric: review-
Details | Formatted Diff | Diff
Patch with Update Copyright Notices (23.10 KB, patch)
2009-06-24 11:40 PDT, Robert Hogan
abarth: review-
Details | Formatted Diff | Diff
New class DRTHistory replacing HistoryManager (23.03 KB, patch)
2009-06-25 11:02 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
diff (22.77 KB, text/plain)
2009-06-25 11:09 PDT, Robert Hogan
no flags Details
Updated patch (22.37 KB, patch)
2009-06-30 15:20 PDT, Robert Hogan
eric: review-
Details | Formatted Diff | Diff
Updated patch (22.55 KB, patch)
2009-08-09 04:41 PDT, Robert Hogan
abarth: review-
Details | Formatted Diff | Diff
Update Patch (30.19 KB, patch)
2009-12-04 14:07 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
updated patch (35.19 KB, patch)
2009-12-06 08:35 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Update to skip test for Mac (35.70 KB, patch)
2009-12-06 12:05 PST, Robert Hogan
eric: review-
eric: commit-queue-
Details | Formatted Diff | Diff
Updated Patch (35.35 KB, patch)
2010-02-17 11:08 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Updated Patch (35.27 KB, patch)
2010-02-21 11:33 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Updated Patch (35.18 KB, patch)
2010-02-21 11:50 PST, Robert Hogan
abarth: review-
Details | Formatted Diff | Diff
Work in progress. (24.89 KB, patch)
2010-04-01 14:43 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
This patch gets all the drawing code changed properly (except for SVG fill and stroke). (62.62 KB, patch)
2010-04-02 12:37 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Fix collapsed border drawing to propagate colors from the original elements participating in the collapse (81.04 KB, patch)
2010-04-02 14:52 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
This patch gets the feature working! (86.28 KB, patch)
2010-04-05 12:02 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Add SVG fill/stroke support (92.58 KB, patch)
2010-04-05 14:22 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Ready for a first review pass (101.61 KB, patch)
2010-04-05 15:38 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch that fixes link hashing for <a href=""> (2.53 KB, patch)
2010-04-07 08:39 PDT, Dave Hyatt
aroben: review-
Details | Formatted Diff | Diff
Patch including ChangeLogs and tests (144.10 KB, patch)
2010-04-07 13:26 PDT, Dave Hyatt
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2009-03-02 13:46:55 PST
http://ha.ckers.org/weird/CSS-history.cgi demonstrates how malicious websites can use the 'visited' element in CSS stylesheets to inspect the user's recent browsing history. 

The attached patch introduces a webcore setting 'CSSKeepVisitedLinksPrivate' that allows webkit browsers to disable this behaviour.
Comment 1 Robert Hogan 2009-03-02 13:48:28 PST
Created attachment 28196 [details]
CSSKeepVisitedLinksPrivate patch

Tested with Arora.
Comment 2 Eric Seidel (no email) 2009-05-19 22:36:01 PDT
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().
Comment 3 Robert Hogan 2009-06-22 12:14:53 PDT
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 4 Adam Barth 2009-06-22 23:42:56 PDT
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.
Comment 5 Robert Hogan 2009-06-23 15:29:53 PDT
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 6 Adam Barth 2009-06-23 15:39:45 PDT
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?
Comment 7 Robert Hogan 2009-06-23 15:59:25 PDT
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 8 Adam Barth 2009-06-23 17:08:31 PDT
Comment on attachment 31753 [details]
Updated patch with suggested changes

Awesome!  Thanks for fixing the nits.
Comment 9 Sam Weinig 2009-06-23 23:51:03 PDT
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 10 Eric Seidel (no email) 2009-06-23 23:52:02 PDT
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.
Comment 11 Eric Seidel (no email) 2009-06-23 23:52:34 PDT
Oh, nm.  I never landed it. :)  So not rolling it out... but r-ing it. :)
Comment 12 Maciej Stachowiak 2009-06-23 23:52:34 PDT
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-.
Comment 13 Robert Hogan 2009-06-24 11:40:49 PDT
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 14 Adam Barth 2009-06-25 00:29:09 PDT
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.)
Comment 15 Robert Hogan 2009-06-25 05:44:12 PDT
(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?

Comment 16 Adam Barth 2009-06-25 08:49:48 PDT
> 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.
Comment 17 Benjamin Meyer 2009-06-25 10:44:24 PDT
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.
Comment 18 Robert Hogan 2009-06-25 11:02:30 PDT
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.
Comment 19 Robert Hogan 2009-06-25 11:09:18 PDT
Created attachment 31867 [details]
diff

as mentioned in previous comment
Comment 20 Eric Seidel (no email) 2009-06-30 03:04:07 PDT
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 21 Dave Hyatt 2009-06-30 12:09:56 PDT
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?
Comment 22 Robert Hogan 2009-06-30 14:42:19 PDT
(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.
Comment 23 Robert Hogan 2009-06-30 15:20:15 PDT
Created attachment 32098 [details]
Updated patch

Updated to remove redundant check.
Comment 24 Eric Seidel (no email) 2009-08-07 09:26:11 PDT
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.
Comment 25 Kenneth Rohde Christiansen 2009-08-07 10:44:44 PDT
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.
Comment 26 Robert Hogan 2009-08-09 04:41:07 PDT
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.
Comment 27 Eric Seidel (no email) 2009-08-14 17:27:18 PDT
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
Comment 28 Holger Freyther 2009-08-14 18:36:47 PDT
(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.
Comment 29 Kent Tamura 2009-08-23 22:31:38 PDT
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 30 Eric Seidel (no email) 2009-09-09 11:14:56 PDT
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. ;)
Comment 31 Robert Hogan 2009-09-21 13:19:54 PDT
> (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?
Comment 32 Simon Hausmann 2009-09-27 13:55:04 PDT
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?
Comment 33 Robert Hogan 2009-09-28 12:20:10 PDT
(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 34 Adam Barth 2009-10-13 12:55:34 PDT
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.
Comment 35 Robert Hogan 2009-10-22 13:47:50 PDT
(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)
Comment 36 Robert Hogan 2009-12-04 14:07:22 PST
Created attachment 44334 [details]
Update Patch

Updated patch adding preference and unit tests to gtk, qt, and win ports.
Comment 37 Simon Hausmann 2009-12-04 15:47:54 PST
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? :)
Comment 38 Robert Hogan 2009-12-06 08:35:49 PST
Created attachment 44360 [details]
updated patch

Updated per Simon's comments - option is now CSSVisitedStyleEnabled and set to true by default for all platforms.
Comment 39 WebKit Review Bot 2009-12-06 08:38:42 PST
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
Comment 40 Robert Hogan 2009-12-06 12:05:08 PST
Created attachment 44364 [details]
Update to skip test for Mac
Comment 41 Darin Adler 2009-12-07 09:59:43 PST
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.
Comment 42 Robert Hogan 2009-12-07 11:10:25 PST
(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 43 Robert Hogan 2009-12-28 12:46:45 PST
Comment on attachment 44364 [details]
Update to skip test for Mac

See comment #42
Comment 44 Eric Seidel (no email) 2009-12-29 00:26:21 PST
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.
Comment 45 Eric Seidel (no email) 2009-12-29 00:26:47 PST
http://webkit-commit-queue.appspot.com/patch/44364 is the EWS status link btw.
Comment 46 Eric Seidel (no email) 2010-02-01 15:19:04 PST
Comment on attachment 44364 [details]
Update to skip test for Mac

This patch does not apply and appears abandoned.  r-
Comment 47 Robert Hogan 2010-02-17 11:08:02 PST
Created attachment 48915 [details]
Updated Patch

God loves a trier!
Comment 48 Eric Seidel (no email) 2010-02-17 14:32:08 PST
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.
Comment 49 Robert Hogan 2010-02-21 11:33:29 PST
Created attachment 49155 [details]
Updated Patch
Comment 50 WebKit Review Bot 2010-02-21 11:37:16 PST
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.
Comment 51 Robert Hogan 2010-02-21 11:50:38 PST
Created attachment 49157 [details]
Updated Patch
Comment 52 Adam Barth 2010-03-15 13:04:56 PDT
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
Comment 53 Dave Hyatt 2010-03-31 12:19:19 PDT
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.
Comment 54 Dave Hyatt 2010-03-31 12:19:52 PDT
Taking the bug.
Comment 55 Dave Hyatt 2010-04-01 10:53:07 PDT
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.
Comment 56 Dave Hyatt 2010-04-01 14:43:22 PDT
Created attachment 52338 [details]
Work in progress.
Comment 57 Dave Hyatt 2010-04-02 12:37:29 PDT
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.
Comment 58 Dave Hyatt 2010-04-02 14:52:45 PDT
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.
Comment 59 Adam Barth 2010-04-02 16:10:15 PDT
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
Comment 60 Dave Hyatt 2010-04-05 12:02:30 PDT
Created attachment 52552 [details]
This patch gets the feature working!
Comment 61 Dave Hyatt 2010-04-05 14:22:20 PDT
Created attachment 52572 [details]
Add SVG fill/stroke support
Comment 62 Dave Hyatt 2010-04-05 14:36:17 PDT
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.
Comment 63 Dave Hyatt 2010-04-05 15:38:39 PDT
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.
Comment 64 Adam Barth 2010-04-05 15:40:45 PDT
Isn't this trivial to test with pixel tests?
Comment 65 WebKit Review Bot 2010-04-05 15:44:55 PDT
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.
Comment 66 Early Warning System Bot 2010-04-05 15:49:13 PDT
Attachment 52582 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1651197
Comment 67 Alexey Proskuryakov 2010-04-05 15:55:43 PDT
<rdar://problem/6407151>
Comment 68 Dave Hyatt 2010-04-07 08:39:50 PDT
Created attachment 52738 [details]
Patch that fixes link hashing for <a href="">
Comment 69 Adam Roben (:aroben) 2010-04-07 08:41:54 PDT
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!
Comment 70 Dave Hyatt 2010-04-07 13:26:30 PDT
Created attachment 52778 [details]
Patch including ChangeLogs and tests
Comment 71 Oliver Hunt 2010-04-08 14:06:47 PDT
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
Comment 72 Dave Hyatt 2010-04-08 14:30:04 PDT
Fixed in r57292.