787597 - Don't try to parse pages with too many elements

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P1)

Description

[Brian Nicholson (:bnicholson)]

Some large pages (e.g., http://en.wikipedia.org/wiki/List_of_j%C5%8Dy%C5%8D_kanji) can cause the reader worker to exceed 75MB when parsing. These pages have considerably more elements than most readable pages; the page I linked to has >27,000, whereas many don't even break 1,000.

A simple check we can do before creating the worker is to count the number of elements in document. If they exceed X, bail immediately and don't try to parse.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Don't do a reader parse for pages with > 3000 elements

This patch limits the number of elements to 3000. There's nothing particularly scientific about this number, but it's a higher than most pages, and considerably lower than those that cause excessive memory use.

On my Droid RAZR, this check takes between 0-5ms on most pages. On the page linked to in comment 0, it takes 34ms. I don't think this is too terrible, especially considering the huge overhead we're preventing.

Regarding the reader check, the only test page that now fails that passed before is: http://code.google.com/p/arc90labs-readability/source/browse/branches/haiti/js/readability.js. But I'm not sure this should even be considered a readable page anyway.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 657494 [details] [diff] [review]
Don't do a reader parse for pages with > 3000 elements

Remove the timing code before landing. We can always tweak the max count if needed.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 657494 [details] [diff] [review]
Don't do a reader parse for pages with > 3000 elements

Review of attachment 657494 [details] [diff] [review]:
-----------------------------------------------------------------

Yep. Remove the timing code before landing.

Comment 4

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e8d7da1bd0b5

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 657494 [details] [diff] [review]
Don't do a reader parse for pages with > 3000 elements

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: large pages can consume huge amounts of memory for the reader mode parsing
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low risk (only prevents large pages from doing a parse)
String or UUID changes made by this patch: none

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8d7da1bd0b5

Comment 7

[Alex Keybl [:akeybl]]

Comment on attachment 657494 [details] [diff] [review]
Don't do a reader parse for pages with > 3000 elements

[Triage Comment]
Approving for all branches given this is a new feature. The worst that could happen here is presumably that Reader Mode isn't offered enough. Good optimization.

Comment 8

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/f3d15d12b091

Comment 9

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-beta/rev/9f461f0aaa56

Comment 10

[Cristian Nicolae (:xti)]

This issue seems to be fixed on all branches (Nightly, Aurora and Beta). Closing bug as verified fixed.

--
Device: Galaxy Note
OS: Android 4.0.4