Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Multiple issues caused by ad iframes on news website #530

Closed
1 of 3 tasks
robertknight opened this issue Sep 1, 2017 · 3 comments
Closed
1 of 3 tasks

Multiple issues caused by ad iframes on news website #530

robertknight opened this issue Sep 1, 2017 · 3 comments

Comments

@robertknight
Copy link
Member

robertknight commented Sep 1, 2017 •
edited
Loading

A user reported an issue when using the client on https://www.theguardian.com/world/2016/nov/09/western-civilisation-appiah-reith-lecture where it would appear to "reload" repeatedly.

Original report: https://hypothesis.zendesk.com/agent/tickets/1407

Digging into the problem, it looks like this page has a large number (~10) of ad-generated iframes which are loaded dynamically while scrolling the page. This causes a number of issues with the client's support for iframes. Several of these are race conditions so they will not reliably reproduce all the time.

  1. Sometimes the client can get injected into an iframe multiple times. I believe this is caused by FrameObserver's _addFrame not immediately registering the iframe as "handled" if the frame has not yet finished loading. Therefore _addFrame can be called for the same frame multiple times, and thus the onFrameAdded callback can be triggered for the same frame multiple times.
  2. The support for multiple iframes in the sidebar code is incomplete in some ways. See the explanation here: All annotations orphaned for no obvious reason #527 (comment)
  3. The iframes into which we are injecting the client consist mostly of a single large image or another iframe, so there isn't really any point enabling annotation for them. We're just burning CPU cycles and using data. We have heuristics to filter the set of iframes into which the client is injected, but these are currently based purely on size. A large image/video ad is big enough to match the criteria.

Capturing the above as a checklist:

  • Client injected into same iframe multiple times
  • Non-deterministic anchoring messaging handling
  • Client injected into iframes unnecessarily
robertknight added a commit that referenced this issue Sep 7, 2017
@robertknight
Require iframes to opt into annotation support
02edfeb 
The client's iframe support is not yet robust enough (see
#530) to enable it
automatically for all iframes on arbitrary web pages.

To support the needs of EPUB viewers and others in the meantime while
preventing problems on eg. pages with larger numbers of iframed ads,
require the publisher to opt iframes into annotation by adding the
"enable-annotation" attribute to them.
robertknight added a commit that referenced this issue Sep 7, 2017
@robertknight
Require iframes to opt into annotation support
1dbd490 
The client's iframe support is not yet robust enough (see
#530) to enable it
automatically for all iframes on arbitrary web pages.

To support the needs of EPUB viewers and others in the meantime while
preventing problems on eg. pages with larger numbers of iframed ads,
require the publisher to opt iframes into annotation by adding the
"enable-annotation" attribute to them.
@robertknight robertknight mentioned this issue Sep 7, 2017
Merged
@dwhly
Copy link
Member

dwhly commented Sep 9, 2017

Also happening on this page https://www.wired.com/story/how-apple-finally-made-siri-sound-more-human?mbid=nl_090717_daily

Load in FF, activate bookmarklet, as ads cycle, they will cause annotation cards to load and unload over and over again. cc @heatherstaines

@robertknight
Copy link
Member Author

Hypothesis v1.40 works around this issue for now by limiting annotation of iframes to those which are explicitly opted in by the site owner via an enable-annotation attribute on the <iframe> element.

The underlying issues mentioned above still exist in the codebase and I'll file separate issues for them.

@robertknight robertknight mentioned this issue Sep 11, 2017
Closed
@robertknight
Copy link
Member Author

Closing in favour of #539 and #540.

@robertknight robertknight mentioned this issue Aug 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@robertknight @dwhly