Open Bug 557579 Opened 10 years ago Updated Last year

do restyle when history lookup finishes regardless of whether links are visited

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

ASSIGNED
Tracking Status
blocking2.0 --- -

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [visited-privacy][pixel-stealing])

Attachments

(1 file)

An additional (and probably better privacy-wise) fix for bug 557287 is to make the completion of history lookups trigger restyles whether or not links are visited.  This would remove one of the main ways that presence in history might be detectable, although I have no idea how to detect it through this method.

In particular, at the top of VisitedQuery::HandleCompletion we make a notification that goes into content/layout only when links are visited.  We'd want to do the same amount of restyling work whether or not links are visited.
That basically needs a significant change to the IHistory API, right?
IHistory says it won't notify if you are not visited, and we'd have to fix consumers of that.  I know Link asserts this in at least one place, and I'm not sure how many other invariants we'd be breaking.  At least they are all covered by a bunch of assertions.
Should I have posted bug 600025 comment 6 here instead?
Blocks: 600025
blocking, because it blocks bug 600025.  And beta8 because we probably want any API changes that need to happen to happen sooner rather than later.
blocking2.0: --- → beta8+
Attached patch patchSplinter Review
Here's my current work-in-progress.
(In reply to comment #5)
>  if (aIsVisited) {
>    // All the registered nodes can now be removed for this URI.
>    mObservers.RemoveEntry(aURI);
>  }

Doing anything conditionally on the visitedness of a link seems dangerous. I haven't tried this, but suppose the attacker wants to know if the user has been to example.com. An attacker could set the href of a hyperlink to example.com, then navigate to example.com in a hidden iframe. If a MozAfterPaint event fires on the document where the hyperlink lives, you'd know the user hasn't been to www.example.com yet. Otherwise, they have.
(In reply to comment #6)
> (In reply to comment #5)
> >  if (aIsVisited) {
> >    // All the registered nodes can now be removed for this URI.
> >    mObservers.RemoveEntry(aURI);
> >  }
> 
> Doing anything conditionally on the visitedness of a link seems dangerous. I
> haven't tried this, but suppose the attacker wants to know if the user has been
> to example.com. An attacker could set the href of a hyperlink to example.com,
> then navigate to example.com in a hidden iframe. If a MozAfterPaint event fires
> on the document where the hyperlink lives, you'd know the user hasn't been to
> www.example.com yet. Otherwise, they have.

Right.

But fixing that, given the current situation, would create an even worse bug, where a Web site (while open and in a visible window, I think...where how well we compute a window being visible varies by platform) can track *every* visit you make to another page.

So I think really fixing everything we'd want to fix would mean doing something like (in addition to the patch above):
 * sending notifications of link state every time a page is visited (I'm not sure how big a change to the history code would be required to do this, but I'm probably not the person best suited to do it)
 * changing link-visitedness restyling to delay until the next time the page or a part thereof gets repainted after some period when the entire page was hidden

This is sounding big enough that I no longer think it's realistic to fix in time for Firefox 4.
(In reply to comment #7)
>  * sending notifications of link state every time a page is visited (I'm not
> sure how big a change to the history code would be required to do this, but I'm
> probably not the person best suited to do it)
Getting a notification for every visit is easy.  Storing every link so we can always notify is not so easy and would break a few of the invariants in the History.cpp code.
I think what I think we should do for Firefox 4 is:
 * take this patch (perhaps with revisions)
 * disable MozAfterPaint for content by default (but have a pref that allows people who want to do performance testing with it to enable it)
Attachment #482716 - Flags: review?(bzbarsky)
Attachment #482716 - Flags: review?(sdwilsh)
Comment on attachment 482716 [details] [diff] [review]
patch

Document why the state passed to ContentStatesChanged is not the actual state change but both of the visited states?

r=me with that.
Attachment #482716 - Flags: review?(bzbarsky) → review+
(In reply to comment #9)
> I think what I think we should do for Firefox 4 is:
>  * take this patch (perhaps with revisions)
>  * disable MozAfterPaint for content by default (but have a pref that allows
> people who want to do performance testing with it to enable it)

This seems like a reasonable approach for the Firefox 4 time frame. I created bug 608030 for the second bullet.
Comment on attachment 482716 [details] [diff] [review]
patch

>+  doc->ContentStatesChanged(content, nsnull,
>+                            NS_EVENT_STATE_VISITED | NS_EVENT_STATE_UNVISITED);
Please add a comment saying why we do this.

> void
>-History::NotifyVisited(nsIURI* aURI)
>+History::NotifyVisited(nsIURI* aURI, bool aIsVisited)
nit: second parameter on the next line please

>   /**
>    * Notifies about the visited status of a given URI.
>    *
>    * @param aURI
>    *        The URI to notify about.
>    */
>-  void NotifyVisited(nsIURI *aURI);
>+  void NotifyVisited(nsIURI *aURI, bool aIsVisited);
Please also update the documentation here.

r=sdwilsh with those changes.
Attachment #482716 - Flags: review?(sdwilsh) → review+
blocking2.0: beta8+ → betaN+
How about this:
* Track the number of pending link-visitedness queries for a page.
* When a link-visitedness query is resolved, don't restyle the link if there are still outstanding queries.
* When the number of outstanding queries reaches zero, restyle every link in the page for which a link-visitedness query was performed.
I think we could solve comment #7 this way:
-- After the initial visitedness query has been resolved for a link, a notification that the link is visited doesn't trigger anything other than setting a flag on the link to indicate that the visitedness state should be honoured from the next time we fully repaint the link.
-- An easy way to define "fully repaint" would be when DLBI detects that no display item for the link has content retained from a previous paint.
Blocks: 773338
Whiteboard: [visited-privacy]
Whiteboard: [visited-privacy] → [visited-privacy][pixel-stealing]
You need to log in before you can comment on or make changes to this bug.