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

CSS stylesheets that error out block the parser and the load event #11535

Closed
jdm opened this issue Jun 1, 2016 · 4 comments
Closed

CSS stylesheets that error out block the parser and the load event #11535

jdm opened this issue Jun 1, 2016 · 4 comments
Labels

Comments

@jdm
Copy link
Member

@jdm jdm commented Jun 1, 2016

This is the cause of the 163.com testcase in tp5 failing to load. The early return in StylesheetContext::response_complete prevents us from removing the entry in the DocumentLoader's blocking loads list for a stylesheet that fails to load, and once that is corrected the parser refuses to unblock until the number of blocking stylesheets is also decreased. This diff makes the page load correctly for me.

@shinglyu, you may want to try running the full set with that diff applied and see how many other sites improve.

@jdm jdm added the A-content/dom label Jun 1, 2016
@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jun 2, 2016

I run the patched version on TP5 and got the this:

========================================
Total 69 tests; 48 succeeded, 21 failed.

Failure summary:
 - http://localhost:8000/page_load_test/naver.com/www.naver.com/index.html
 - http://localhost:8000/page_load_test/noimpactman.typepad.com/noimpactman.typepad.com/index.html
 - http://localhost:8000/page_load_test/163.com/www.163.com/index.html
 - http://localhost:8000/page_load_test/thepiratebay.org/thepiratebay.org/top/201.html
 - http://localhost:8000/page_load_test/people.com.cn/people.com.cn/index.html
 - http://localhost:8000/page_load_test/reuters.com/www.reuters.com/index.html
 - http://localhost:8000/page_load_test/imdb.com/www.imdb.com/title/tt1099212/index.html
 - http://localhost:8000/page_load_test/homeway.com.cn/www.hexun.com/index.html
 - http://localhost:8000/page_load_test/indiatimes.com/www.indiatimes.com/index.html
 - http://localhost:8000/page_load_test/wsj.com/online.wsj.com/home-page.html
 - http://localhost:8000/page_load_test/slideshare.net/www.slideshare.net/jameswillamor/lolcats-in-popular-culture-a-historical-perspective.html
 - http://localhost:8000/page_load_test/chinaz.com/chinaz.com/index.html
 - http://localhost:8000/page_load_test/bild.de/www.bild.de/index.html
 - http://localhost:8000/page_load_test/xinhuanet.com/xinhuanet.com/index.html
 - http://localhost:8000/page_load_test/media.photobucket.com/media.photobucket.com/image/funny%20gif/findstuff22/Best%20Images/Funny/funny-gif1.jpg@o=1.html
 - http://localhost:8000/page_load_test/xunlei.com/xunlei.com/index.html
 - http://localhost:8000/page_load_test/store.apple.com/store.apple.com/us@mco=Nzc1MjMwNA.html
 - http://localhost:8000/page_load_test/digg.com/digg.com/news/story/New_logo_for_Mozilla_Firefox_browser.html
 - http://localhost:8000/page_load_test/mashable.com/mashable.com/index.html
 - http://localhost:8000/page_load_test/sohu.com/www.sohu.com/index.html
 - http://localhost:8000/page_load_test/ifeng.com/ifeng.com/index.html
========================================

(The total tests count is a little bit off, because I also counted sub pages e.g. iframes)

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jun 2, 2016

Comparing to #11087 , this fixed 3 pages, breaks 1 page (xunlei.com). photobucket failed, but this behavior started a few weeks ago, so it's not caused by your patch.

This is the diff

% diff -u old.txt new_sorted.txt                                                                                                                           1 ↵
--- old.txt 2016-06-02 13:55:07.692495607 +0800
+++ new_sorted.txt  2016-06-02 13:55:28.920567697 +0800
@@ -1,14 +1,13 @@
 http://localhost:8000/page_load_test/163.com/www.163.com/index.html
-http://localhost:8000/page_load_test/aljazeera.net/aljazeera.net/portal.html
 http://localhost:8000/page_load_test/bild.de/www.bild.de/index.html
 http://localhost:8000/page_load_test/chinaz.com/chinaz.com/index.html
 http://localhost:8000/page_load_test/digg.com/digg.com/news/story/New_logo_for_Mozilla_Firefox_browser.html
-http://localhost:8000/page_load_test/guardian.co.uk/www.guardian.co.uk/index.html
 http://localhost:8000/page_load_test/homeway.com.cn/www.hexun.com/index.html
 http://localhost:8000/page_load_test/ifeng.com/ifeng.com/index.html
 http://localhost:8000/page_load_test/imdb.com/www.imdb.com/title/tt1099212/index.html
 http://localhost:8000/page_load_test/indiatimes.com/www.indiatimes.com/index.html
 http://localhost:8000/page_load_test/mashable.com/mashable.com/index.html
+http://localhost:8000/page_load_test/media.photobucket.com/media.photobucket.com/image/funny%20gif/findstuff22/Best%20Images/Funny/funny-gif1.jpg@o=1.html
 http://localhost:8000/page_load_test/naver.com/www.naver.com/index.html
 http://localhost:8000/page_load_test/noimpactman.typepad.com/noimpactman.typepad.com/index.html
 http://localhost:8000/page_load_test/people.com.cn/people.com.cn/index.html
@@ -17,6 +16,6 @@
 http://localhost:8000/page_load_test/sohu.com/www.sohu.com/index.html
 http://localhost:8000/page_load_test/store.apple.com/store.apple.com/us@mco=Nzc1MjMwNA.html
 http://localhost:8000/page_load_test/thepiratebay.org/thepiratebay.org/top/201.html
-http://localhost:8000/page_load_test/uol.com.br/www.uol.com.br/index.html
 http://localhost:8000/page_load_test/wsj.com/online.wsj.com/home-page.html
 http://localhost:8000/page_load_test/xinhuanet.com/xinhuanet.com/index.html
+http://localhost:8000/page_load_test/xunlei.com/xunlei.com/index.html
@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jun 2, 2016

@jdm I also noticed that when servo got stuck, the server log usually says it serving a gif file. We could probably check if gif loading or rendering is slow.

@jdm
Copy link
Member Author

@jdm jdm commented Jun 2, 2016

Huh, 163.com didn't finish loading? That's the one that I was testing.

bors-servo added a commit that referenced this issue Jun 29, 2016
Avoid stalled pages containing unreachable stylesheets

This hits some of the pages in the performance testsuite, and leads to a poor user experience since the document load event is never dispatched and the page never finishes parsing.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11535 (github issue number if applicable).
- [X] There are tests for these changes OR

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11913)
<!-- Reviewable:end -->
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.

2 participants
You can’t perform that action at this time.