Closed
Bug 557579
Opened 14 years ago
Closed 3 years ago
do restyle when history lookup finishes regardless of whether links are visited
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
DUPLICATE
of bug 1506842
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Whiteboard: [visited-privacy][pixel-stealing])
Attachments
(1 file)
15.57 KB,
patch
|
bzbarsky
:
review+
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
That basically needs a significant change to the IHistory API, right?
Comment 2•14 years ago
|
||
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.
Comment 3•14 years ago
|
||
Should I have posted bug 600025 comment 6 here instead?
Assignee | ||
Comment 4•14 years ago
|
||
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+
Assignee | ||
Comment 5•14 years ago
|
||
Here's my current work-in-progress.
Comment 6•14 years ago
|
||
(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.
Assignee | ||
Comment 7•14 years ago
|
||
(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.
Comment 8•14 years ago
|
||
(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.
Assignee | ||
Comment 9•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #482716 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #482716 -
Flags: review?(sdwilsh)
Comment 10•14 years ago
|
||
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+
Comment 11•14 years ago
|
||
(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 12•14 years ago
|
||
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+
Updated•14 years ago
|
blocking2.0: beta8+ → betaN+
Assignee | ||
Comment 13•14 years ago
|
||
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=e250978a21be&newRev=9fef933bb7bf&tests=a11y,tdhtml,tdhtml_nochrome,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tgfx,tgfx_nochrome,tscroll,tsvg,tsvg_opacity,ts,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen&submit=true suggests this causes a 4%-9% tp4 regression. I'm not sure what to do about that.
blocking2.0: 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.
Updated•6 years ago
|
Whiteboard: [visited-privacy] → [visited-privacy][pixel-stealing]
Comment 16•3 years ago
|
||
Bug 1506842 fixed this: https://searchfox.org/mozilla-central/rev/71621bfa47a371f2b1ccfd33c704913124afb933/dom/base/Link.cpp#110-115
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•