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

Error when top level frame and iframe on a page embed the Hypothesis client #187

Closed
robertknight opened this issue Jan 13, 2017 · 9 comments
Closed

Error when top level frame and iframe on a page embed the Hypothesis client #187

robertknight opened this issue Jan 13, 2017 · 9 comments

Comments

@robertknight
Copy link
Member

A page which embeds Hypothesis in the top-level frame and a child iframe cause an error to be reported in Firefox. The same scenario in Chrome did not result in an error in my testing but there have been Sentry reports (eg. https://sentry.io/hypothesis/prod-client/issues/163858937/) with this error from Chrome.

This was originally reproduced by visiting http://tiki.oneworld.org/plastic/plastic.html in Firefox and clicking on the "Click here for my 3-minute slideshow" link.

Steps to reproduce

  1. Save the files below as index.html and viewer.html
  2. Open index.html in Firefox
  3. Check console

Expected: No error
Actual: A second Discovery server .... error. See https://sentry.io/hypothesis/prod-client/issues/163858937/

index.html

<html>
<head>
  <meta charset="UTF-8">
</head>
<body>
  <script async defer src="https://hypothes.is/embed.js"></script>
  <iframe src="viewer.html"></iframe>
</body>
</html>

viewer.html

<html>
<head>
  <meta charset="UTF-8">
</head>
<body>
  <script async defer src="https://hypothes.is/embed.js"></script>
</body>
</html>
@sean-roberts
Copy link
Contributor

sentry points to the example on this site: http://tiki.oneworld.org/plastic/plastic.html
click the 3 min video link to trigger a popup + iframe

@robertknight
Copy link
Member Author

Sentry issue: CLIENT-K0K

@robertknight
Copy link
Member Author

The most common cause of this appears to be when visiting a page through Via which has iframes. For example visit https://via.hypothes.is/https://www.theverge.com/circuitbreaker/2018/8/2/17645372/samsung-galaxy-note-9-leak-accidental and scroll down so that the YouTube video embed is visible.

There is code in https://github.com/hypothesis/via/blob/03efec3ac301c38ecb1063bec5d802a51a9569ca/templates/banner.html#L12 that is supposed to prevent this but it doesn't work. Note that since all proxied pages are on the same origin (via.hypothes.is) this issue applies: #249

@ajpeddakotla
Copy link

@robertknight is it possible that this bug is causing some or perhaps even a majority of the via errors we're seeing?

@ajpeddakotla
Copy link

Note from Robert: It is an issue with the client that happens to manifest itself in the context of via. We might want to put a patch in via to prevent spamming on sentry (which is what was seen previously with this issue).

@robertknight
Copy link
Member Author

This issue caused us to have to turn off Sentry error reporting for the client a while back because it was happening so often, and we didn't have appropriate rate limiting from the client set up: #759

@robertknight robertknight mentioned this issue Jul 1, 2019
Closed
@robertknight
Copy link
Member Author

I started putting together a prototype a while back to explore how to simplify the way the sidebar and content frames discover each other. It can be found at https://github.com/hypothesis/client/tree/new-discovery-prototype/discovery-prototype.

@LMS007 LMS007 mentioned this issue Mar 18, 2020
Closed
@lyzadanger
Copy link
Contributor

@robertknight Can you help me understand how this one differs from #249 (if it does)?

@klemay klemay mentioned this issue Nov 9, 2020
Closed
@robertknight robertknight mentioned this issue Feb 4, 2021
Closed
@esanzgar esanzgar mentioned this issue Jun 30, 2021
Closed
@robertknight robertknight mentioned this issue Jul 21, 2021
Merged
2 tasks
@robertknight
Copy link
Member Author

This was fixed initially by #3599 and then #3611 replaced the whole mechanism that is used by frames to discover each other. There is ongoing work in #3533 which will lift the restriction that host and guest frames must be same-origin.

In the client's development server there is a manual test page for this at http://localhost:3000/document/multi-frames.

@robertknight robertknight mentioned this issue Aug 11, 2021
Closed
1 task
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
4 participants
@robertknight @lyzadanger @sean-roberts @ajpeddakotla