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.

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

Twitter won't load - stuck in endless refresh prompt #26032

Closed
dralley opened this issue Mar 25, 2020 · 5 comments
Closed

Twitter won't load - stuck in endless refresh prompt #26032

dralley opened this issue Mar 25, 2020 · 5 comments

Comments

@dralley
Copy link
Contributor

@dralley dralley commented Mar 25, 2020

No error or explicit failure. Clicking the refresh text refreshes the page but it just asks to refresh again afterwards, endlessly.

Screenshot from 2020-03-25 10-03-35

@dralley
Copy link
Contributor Author

@dralley dralley commented Mar 25, 2020

re: #18818

@dralley dralley changed the title Twitter won't load - stuck in endless "please refresh" prompt Twitter won't load - stuck in endless refresh prompt Mar 25, 2020
@jdm jdm mentioned this issue Mar 30, 2020
4 of 15 tasks complete
@CYBAI
Copy link
Collaborator

@CYBAI CYBAI commented Jun 4, 2020

Thanks to the --load-script-source option! We're able to debug twitter locally now!

Finally figured out why twitter can't be rendered correctly in Servo now.

Following is how I tried to debug it and the root cause in the end.


Debugging Process

This is currently how twitter is rendered in Servo.

image

The twitter error message says, Something went wrong, but don't fret - it's not your fault.
With grepping from unminified-js folder, we can know the message is defined in abs.twimg.com/responsive-web/web/i18n-rweb/en.f1d67e74.js and it's equal to aaf2506f.

image

With grepping aaf2506f, we can know it's the dJGd react component because it has componentDidCatch for showing the error message.

image

So, I added console.log(t.componentStack) into the componentDidCatch function; then, it shows that the error is from circle element inside ActivityIndicator.

Stacktrace of the react component
    in circle
    in svg
    in div
    in View
    in div
    in View
    in ActivityIndicator
    in @twitter/ActivityIndicator
    in div
    in View
    in t
    in n
    in div
    in View
    in div
    in View
    in div
    in View
    in t
    in div
    in View
    in @twitter/Responsive
    in @twitter/Container
    in div
    in View
    in header
    in View
    in t
    in i
    in _
    in routerWithMatchPropRemoved(Connect(i))
    in withRouter(routerWithMatchPropRemoved(Connect(i)))
    in div
    in View
    in @twitter/Responsive
    in t
    in t
    in u
    in ForwardRef
    in ForwardRef
    in t
    in t
    in t
    in _
    in t
    in u
    in ForwardRef
    in ForwardRef
    in t
    in Unknown
    in t
    in t
    in r
    in routerWithMatchPropRemoved(r)
    in withRouter(routerWithMatchPropRemoved(r))
    in t
    in t
    in t
    in _
    in s
    in t
    in div
    in View
    in div
    in View
    in r

Digging deeper into the ActivityIndicator component, we can find it's the tyyN react component in abs.twimg.com/responsive-web/web/vendors~main.19622e04.js. The circle elements and svg element are there.

image

I tried several ways to see if twitter can be rendered correctly and following 3 kinds work fine to render a broken layout twitter

  1. Change the svg to div
  2. Remove the circle components
  3. Remove style props of circle

The 3rd one of removing style just made me notice if it's related to style.

Then, I tried to create a minimized test case on codesandbox (here).

Luckily and thankfully, codesandbox uses a dev build of react and it will show the full stacktrace for why it fails.

image


Find the root cause

https://github.com/facebook/react/blob/4c7036e807fa18a3e21a5182983c7c0f05c5936e/packages/react-dom/src/shared/CSSPropertyOperations.js#L52-L85

This is the setValueForStyles function in react-dom library.

It gets style from the node (an element) and will set the style value to the style.

However, we don't support style for SVGElement yet so it just throws error while setting a value to undefined in line 82.


TL;DR

We don't support style for SVGElement yet. React will use it to mutate styles so it will fail to mutate in Servo currently due to assigning a value to undefined.

bors-servo added a commit that referenced this issue Jun 4, 2020
Introduce ElementCSSInlineStyle for SVGElement

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26777 and fix #26032 and fix #21990
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo bors-servo closed this in 59ec134 Jun 4, 2020
@atouchet
Copy link
Contributor

@atouchet atouchet commented Jun 5, 2020

This still isn't working for me in the latest nightly. Am I doing something wrong?

@CYBAI
Copy link
Collaborator

@CYBAI CYBAI commented Jun 5, 2020

The issue is related to SVG so you will need to enable SVG to make it work.

./mach run --pref dom.svg.enabled https://twitter.com
@atouchet
Copy link
Contributor

@atouchet atouchet commented Jun 5, 2020

Ah, that does appear to get it to load. The site has major visual issues when it loads though. I don't remember it being nearly that bad previously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
@dralley @CYBAI @atouchet and others
You can’t perform that action at this time.