779796 - Move readability check to web worker

Status: VERIFIED FIXED

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

Description

[Lucas Rocha (:lucasr)]

This way we can run the full readability check without blocking Gecko in any way.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Implement JSDOMParser

The DOM cannot be used in web workers, so we need our own implementation to be used by Readability.js. This is a pretty minimal implementation that doesn't have some of the overhead from a complete DOM (such as live NodeLists).

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Replace Reader checks with full parses

Since the JSDOMParser allows us to do relatively quick parses in a separate thread, we can just do a full parse for every page visited. This provides two advantages over the existing check:
1) There will be no more false positives (fixing bug 767956)
2) Parsed pages will be immediately available to read or add to the reading list

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Update Readability.js to be compatible with JSDOMParser

grabArticle() manipulates elements in the document as it iterates through the allElements NodeList. Since JSDOMParser doesn't support live NodeLists, we have to manually keep the allElements array updated with changes. It's a bit messier, but faster.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Move Readability parsing to a web worker

Moves the parsing to a web worker. This includes a check to make sure the URL of the page hasn't changed during the parse, which should fix bug 779219.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Part 5: Remove XPCOM-related code from Readability.js

Carried over from bug 779389. I'm not particularly excited about the relative URL change, but since workers cannot use XPCOM, we don't have tons of options. An alternative might be to message the main thread to do the relative URL fixup, but that could make things messier and slower.

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Part 6: Remove article generator function in Readability.js

No longer needed since we're in a separate thread.

Comment 8

[Brian Nicholson (:bnicholson)]

The JSDOMParser cannot handle improperly formed HTML, so bug 777966 needs to land before or at the same time as this one.

Comment 9

[Brian Nicholson (:bnicholson)]

We'll want to cache all articles so they're readily available as described in comment 3. I've filed bug 780519 for this.

Comment 10

[Brian Nicholson (:bnicholson)]

Here's a test build containing changes from this bug and bug 777966: http://dl.dropbox.com/u/35559547/fennec-reader-worker.apk

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 649146 [details] [diff] [review]
Part 6: Remove article generator function in Readability.js

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

::: mobile/android/chrome/content/Readability.js
@@ +386,5 @@
>     *
>     * @param page a document to run upon. Needs to be a full document, complete with body.
>     * @return Element
>    **/
> +  _grabArticle: function (page, callback) {

grabArticle() doesn't need to be async anymore, does it? You should probably return the result directly instead of using a callback then.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 649145 [details] [diff] [review]
Part 5: Remove XPCOM-related code from Readability.js

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

Nice one. Just want to see a new cleaned up version of this patch.

::: mobile/android/chrome/content/Readability.js
@@ +98,5 @@
>     * @param Element
>     * @return void
>     */
>    _fixRelativeUris: function(articleContent) {
> +    let spec = this._uri.spec;

You don't use spec below, do you?

@@ +101,5 @@
>    _fixRelativeUris: function(articleContent) {
> +    let spec = this._uri.spec;
> +    let scheme = this._uri.scheme;
> +    let prePath = this._uri.prePath;
> +    let pathBase = this._uri.pathBase;

Seem like you should bail (or use "#" as the link) if pathBase is undefined? Error building pathBase in browser.js simply logs the exception but still runs the readability parsing.

@@ +117,5 @@
> +    }
> +
> +    function toAbsoluteURI(uri) {
> +      // If this is already an absolute URI, return it.
> +      if (/^[a-zA-Z]+:/.test(uri))

Isn't it safer if you make the uri doesn't start with "."? Something like "/^[^\.]/?

@@ +122,5 @@
> +        return uri;
> +
> +      // If this page isn't one of the whitelisted schemes, don't resolve its
> +      // relative URLs.
> +      if (!shouldResolveRelativeURIs)

This check should probably come first.

Comment 13

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #12)
> Comment on attachment 649145 [details] [diff] [review]
> Part 5: Remove XPCOM-related code from Readability.js
> 
> Review of attachment 649145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice one. Just want to see a new cleaned up version of this patch.
> 
> ::: mobile/android/chrome/content/Readability.js
> @@ +98,5 @@
> >     * @param Element
> >     * @return void
> >     */
> >    _fixRelativeUris: function(articleContent) {
> > +    let spec = this._uri.spec;
> 
> You don't use spec below, do you?

Nope, thanks.

> @@ +101,5 @@
> >    _fixRelativeUris: function(articleContent) {
> > +    let spec = this._uri.spec;
> > +    let scheme = this._uri.scheme;
> > +    let prePath = this._uri.prePath;
> > +    let pathBase = this._uri.pathBase;
> 
> Seem like you should bail (or use "#" as the link) if pathBase is undefined?
> Error building pathBase in browser.js simply logs the exception but still
> runs the readability parsing.

I think the only way getting the pathBase can fail is if we're looking at an about: page. We shouldn't even be running the worker on about: pages since we do schemeIs checks in parseDocumentFromTab(). Since getting the pathBase shouldn't ever fail, perhaps we should just return immediately if the exception is caught in browser.js? In fact, we can probably just wrap all of parseInWorker() in a try/catch, and return null for any errors.

> @@ +117,5 @@
> > +    }
> > +
> > +    function toAbsoluteURI(uri) {
> > +      // If this is already an absolute URI, return it.
> > +      if (/^[a-zA-Z]+:/.test(uri))
> 
> Isn't it safer if you make the uri doesn't start with "."? Something like
> "/^[^\.]/?

I don't follow what you mean. This regex is trying to detect an absolute URL (i.e., whether or not the URL string starts with a scheme). If the URL is "foo", that should be relative; if the URL is "http://foo", that should be absolute. Why would look for a "."?

With that said, this regex can be hardened a bit more to look for unusual schemes. Looking at section 3.1 of http://www.ietf.org/rfc/rfc2396.txt, I think the complete regex should be:

/^[a-zA-z][a-zA-Z0-9\+\-\.]*:/

> @@ +122,5 @@
> > +        return uri;
> > +
> > +      // If this page isn't one of the whitelisted schemes, don't resolve its
> > +      // relative URLs.
> > +      if (!shouldResolveRelativeURIs)
> 
> This check should probably come first.

I'm wondering if we even want/need this check. Again, we shouldn't even be running the worker on about: (or any other non-whitelisted scheme) pages since we check for them before creating the worker.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Part 5: Remove XPCOM-related code from Readability.js, v2

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: Part 6: Remove article generator function in Readability.js, v2

Makes Readability.js synchronous again.

Comment 16

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Implement JSDOMParser, v2

Fixed to make getAttribute() return the attribute string, and not the Attribute object itself.

Comment 17

[Brian Nicholson (:bnicholson)]

> /^[a-zA-z][a-zA-Z0-9\+\-\.]*:/

I locally fixed the typo here where "A-z" should be "A-Z".

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #13)
> > @@ +101,5 @@
> > >    _fixRelativeUris: function(articleContent) {
> > > +    let spec = this._uri.spec;
> > > +    let scheme = this._uri.scheme;
> > > +    let prePath = this._uri.prePath;
> > > +    let pathBase = this._uri.pathBase;
> > 
> > Seem like you should bail (or use "#" as the link) if pathBase is undefined?
> > Error building pathBase in browser.js simply logs the exception but still
> > runs the readability parsing.
> 
> I think the only way getting the pathBase can fail is if we're looking at an
> about: page. We shouldn't even be running the worker on about: pages since
> we do schemeIs checks in parseDocumentFromTab(). Since getting the pathBase
> shouldn't ever fail, perhaps we should just return immediately if the
> exception is caught in browser.js? In fact, we can probably just wrap all of
> parseInWorker() in a try/catch, and return null for any errors.

Wrapping all parseInWorker in a try/catch block makes sense.

> > @@ +117,5 @@
> > > +    }
> > > +
> > > +    function toAbsoluteURI(uri) {
> > > +      // If this is already an absolute URI, return it.
> > > +      if (/^[a-zA-Z]+:/.test(uri))
> > 
> > Isn't it safer if you make the uri doesn't start with "."? Something like
> > "/^[^\.]/?
> 
> I don't follow what you mean. This regex is trying to detect an absolute URL
> (i.e., whether or not the URL string starts with a scheme). If the URL is
> "foo", that should be relative; if the URL is "http://foo", that should be
> absolute. Why would look for a "."?
> 
> With that said, this regex can be hardened a bit more to look for unusual
> schemes. Looking at section 3.1 of http://www.ietf.org/rfc/rfc2396.txt, I
> think the complete regex should be:
> 
> /^[a-zA-z][a-zA-Z0-9\+\-\.]*:/

The regex you used just looked too naive for something involving URIs :-) Use this complete regex then.

> > @@ +122,5 @@
> > > +        return uri;
> > > +
> > > +      // If this page isn't one of the whitelisted schemes, don't resolve its
> > > +      // relative URLs.
> > > +      if (!shouldResolveRelativeURIs)
> > 
> > This check should probably come first.
> 
> I'm wondering if we even want/need this check. Again, we shouldn't even be
> running the worker on about: (or any other non-whitelisted scheme) pages
> since we check for them before creating the worker.

I like the fact that you're being explicit about the protocols we support in reader. Actually, I'd probably only run the readability check on http and https pages.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 649374 [details] [diff] [review]
Part 5: Remove XPCOM-related code from Readability.js, v2

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

Patch looks good but the order of the patches seems wrong. This patch should probably come before the patch to make move readability to a web worker—otherwise you'll break bisecting because readability.js should have no XPCOM code once it's moved to web worker. So, this should actually be patch #4.

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 649392 [details] [diff] [review]
Part 6: Remove article generator function in Readability.js, v2

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

::: mobile/android/chrome/content/Readability.js
@@ -467,5 @@
>        let doc = this._doc;
>        let stripUnlikelyCandidates = this._flagIsActive(this.FLAG_STRIP_UNLIKELYS);
>        let isPaging = (page !== null ? true: false);
>  
> -      page = page ? page : this._doc.body;

How is this change related? This line should probably be kept here.

::: mobile/android/chrome/content/readerWorker.js
@@ +7,5 @@
>  self.onmessage = function (msg) {
>    let uri = JSON.parse(msg.data.uri);
>    let doc = new JSDOMParser().parse(msg.data.doc);
> +  let article = new Readability(uri, doc).parse();
> +  postMessage(article);

I assume there's no problem in posting a null message here (in case the parse fails).

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking this bug instead of bug 767956 since this is where the action is on fixing the issue.

Comment 22

[Brian Nicholson (:bnicholson)]

Flagging for security review - in particular, Patch 1, which includes a homebrew DOMParser. I don't think there should be any problems since bug 778582 puts the result in an iframe, but it'd be nice to get some feedback in case we're overlooking something.

Comment 23

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

Comment on attachment 649374 [details] [diff] [review]
Part 5: Remove XPCOM-related code from Readability.js, v2

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    let serializedDoc;
>+    let uriJSON;
>+    try {
>+      serializedDoc = new XMLSerializer().serializeToString(doc);
>+      uriJSON = JSON.stringify({

technically, uriJSON is really a string here, just like the serializedDoc. Just name these stringified variables like: _uri and _doc, kinda dumb, I know, but sometimes simple is less confusing.

Comment 24

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

Comment on attachment 649397 [details] [diff] [review]
Part 1: Implement JSDOMParser, v2

>diff --git a/mobile/android/chrome/content/JSDOMParser.js b/mobile/android/chrome/content/JSDOMParser.js

>+  function error(m) {
>+    dump("JSDOMParser error: " + m);
>+  }

how is dump supported?


We need to plan for some tests on this code. Currently, if something bad happens, we just won't get a completed readability check, which isn't too bad. If we start using this code for more than a check, it could be worse. We should also consider profiling this code too.

Comment 25

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #24)
> Comment on attachment 649397 [details] [diff] [review]
> Part 1: Implement JSDOMParser, v2
> 
> >diff --git a/mobile/android/chrome/content/JSDOMParser.js b/mobile/android/chrome/content/JSDOMParser.js
> 
> >+  function error(m) {
> >+    dump("JSDOMParser error: " + m);
> >+  }
> 
> how is dump supported?

dump() is one of the few DOM-related functions available to workers: https://developer.mozilla.org/en-US/docs/DOM/Worker/Functions_available_to_workers

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 649144 [details] [diff] [review]
Part 4: Move Readability parsing to a web worker

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

Nice.

Comment 27

[Lucas Rocha (:lucasr)]

Comment on attachment 649143 [details] [diff] [review]
Part 3: Update Readability.js to be compatible with JSDOMParser

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

Giving r+ but please answer my questions.

::: mobile/android/chrome/content/Readability.js
@@ +431,5 @@
> +       * JSDOMParser returns static node lists, not live ones. When we remove
> +       * an element from the document, we need to manually remove it - and all
> +       * of its children - from the allElements array.
> +       */
> +      function purgeChildren(node) {

Maybe purgeNode() is a more appropriate name?

@@ -430,5 @@
>              unlikelyMatchString.search(this.REGEXPS.okMaybeItsACandidate) === -1 &&
>              node.tagName !== "BODY") {
>              dump("Removing unlikely candidate - " + unlikelyMatchString);
>              node.parentNode.removeChild(node);
> -            nodeIndex -= 1;

Why isn't this assignment needed anymore?

@@ +469,3 @@
>              node.parentNode.replaceChild(newNode, node);
> +            allElements[nodeIndex] = newNode;
> +            purgeChildren(node);

I wonder if you should factor out this index-mangling code into inner function (just like purgeChildren()). Would probably make this code a bit easier to read.

@@ +479,5 @@
>                  continue;
>  
>                if (childNode.nodeType === 3) { // Node.TEXT_NODE
>                  let p = doc.createElement('p');
> +                p.innerHTML = childNode.textContent;

How is this related to this patch?

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 649397 [details] [diff] [review]
Part 1: Implement JSDOMParser, v2

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

This code is begging for unit tests as soon as possible. I don't want to block on this but please file a follow up. We'll definitely need test cases for this code otherwise it will be a pain to spot regressions and determine the cause of certain readability bugs (JSDOMParser, Readability, AboutReader, etc). Giving r+ but please implement the suggestions (and maybe the nits) where applicable.

::: mobile/android/chrome/content/JSDOMParser.js
@@ +289,5 @@
> +    },
> +
> +    get nextSibling() {
> +      if (this.parentNode) {
> +        let childNodes = this.parentNode.childNodes;

Why not simply using childNodes.indexOf(this) here?

@@ +302,5 @@
> +
> +    appendChild: function (child) {
> +      if (child.parentNode) {
> +        child.parentNode.removeChild(child);
> +      }

nit: add empty line here.

@@ +308,5 @@
> +      child.parentNode = this;
> +    },
> +
> +    removeChild: function (child) {
> +      let childNodes = this.childNodes;

Same here: you should probably just use childNodes.indexOf(child) here.

@@ +320,5 @@
> +
> +    replaceChild: function (newNode, oldNode) {
> +      let childNodes = this.childNodes;
> +      for (let i = childNodes.length; --i >= 0;) {
> +        if (childNodes[i] == oldNode) {

Use childNodes.indexOf(oldNode) to get the index.

@@ +345,5 @@
> +    this.childNodes = [];
> +  };
> +
> +  Comment.prototype = {
> +    __proto__: new Node(),

Shouldn't this be more like "__proto__: Node.prototype"? Same applies to all other cases.

@@ +486,5 @@
> +    },
> +
> +    set innerHTML(html) {
> +      let parser = new JSDOMParser();
> +      let node = parser.parse(html);

Should probably do "let length = this.childNotes.length" in a use it in the loop below.

@@ +490,5 @@
> +      let node = parser.parse(html);
> +      for (let i = this.childNodes.length; --i >= 0;) {
> +        this.childNodes[i].parentNode = null;
> +      }
> +      this.childNodes = node.childNodes;

Same here.

@@ +559,5 @@
> +  Style.prototype = {
> +    getStyle: function (styleName) {
> +      let attr = this.node.getAttribute("style");
> +      if (!attr)
> +        return undefined;

nit: add empty line here.

@@ +566,5 @@
> +        let style = styles[i].split(":");
> +        let name = style[0].trim();
> +        if (name == styleName)
> +          return style[1].trim();
> +      }

nit: add empty line here.

@@ +583,5 @@
> +          value = value.substr(0, index).trim() + (next ? " " + value.substr(next).trim() : "");
> +          break;
> +        }
> +        index = next;
> +      } while (index);

nit: add empty line here.

@@ +756,5 @@
> +
> +    /**
> +     * Reads child nodes for the given node.
> +     */
> +    readChildren: function (node) {

nit: I tend to prefer prefixing private methods with '_'. Not a big deal though as we're not doing this consistently in, say, browser.js.

@@ +775,5 @@
> +     * @returns the node
> +     */
> +    readNode: function () {
> +      let c = this.nextChar();
> +      

Remove tab or trailing spaces here.

@@ +795,5 @@
> +        return node;
> +      }
> +
> +      c = this.peekNext();
> +      

Remove tab or trailing spaces here.

Comment 29

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #28)
> Comment on attachment 649397 [details] [diff] [review]
> Part 1: Implement JSDOMParser, v2
> 
> Review of attachment 649397 [details] [diff] [review]:
> 
> ::: mobile/android/chrome/content/JSDOMParser.js
> @@ +289,5 @@
> > +    },
> > +
> > +    get nextSibling() {
> > +      if (this.parentNode) {
> > +        let childNodes = this.parentNode.childNodes;
> 
> Why not simply using childNodes.indexOf(this) here?

Thanks - I forgot that method existed in JS. This change could improve performance a bit.

> @@ +345,5 @@
> > +    this.childNodes = [];
> > +  };
> > +
> > +  Comment.prototype = {
> > +    __proto__: new Node(),
> 
> Shouldn't this be more like "__proto__: Node.prototype"? Same applies to all
> other cases.

I think inheritance in JS is typically done by setting the prototype to an instance of the super class so that the super constructor is called. But since the Node constructor is empty, I guess we can get away with just setting it to Node.prototype in this case.

> @@ +486,5 @@
> > +    },
> > +
> > +    set innerHTML(html) {
> > +      let parser = new JSDOMParser();
> > +      let node = parser.parse(html);
> 
> Should probably do "let length = this.childNotes.length" in a use it in the
> loop below.

Not sure I understand this change; this.childNodes.length only gets used once, when it initializes i in the loop. Why put it into another variable?

Comment 30

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #27)
> Comment on attachment 649143 [details] [diff] [review]
> Part 3: Update Readability.js to be compatible with JSDOMParser
> 
> Review of attachment 649143 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Giving r+ but please answer my questions.
> 
> Maybe purgeNode() is a more appropriate name?

Indeed it is.

> 
> @@ -430,5 @@
> >              unlikelyMatchString.search(this.REGEXPS.okMaybeItsACandidate) === -1 &&
> >              node.tagName !== "BODY") {
> >              dump("Removing unlikely candidate - " + unlikelyMatchString);
> >              node.parentNode.removeChild(node);
> > -            nodeIndex -= 1;
> 
> Why isn't this assignment needed anymore?

With live NodeLists, removing the child here would "shift" the NodeList such that this index refers to the next element, so decrementing nodeIndex allows us to revisit the new node at the same index. allElements is just an Array in JSDOMParser, though, so when we remove/purge this node, this element at this index will just be undefined if we try to look at it again.

> @@ +479,5 @@
> >                  continue;
> >  
> >                if (childNode.nodeType === 3) { // Node.TEXT_NODE
> >                  let p = doc.createElement('p');
> > +                p.innerHTML = childNode.textContent;
> 
> How is this related to this patch?

I could've added a getter in JSDOMParser for nodeValue that just returned textContent, but since this is the only place nodeValue is used in Readability.js, I figured it could just use textContent directly. I think this should also be more efficient since this loop is executed a lot in the code, and getters are more costly than properties.

Comment 31

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c2b13e9506a0
http://hg.mozilla.org/integration/mozilla-inbound/rev/4ed72e80519c
http://hg.mozilla.org/integration/mozilla-inbound/rev/e89419a3b021
http://hg.mozilla.org/integration/mozilla-inbound/rev/b5efa2cc01f5
http://hg.mozilla.org/integration/mozilla-inbound/rev/04f2152643e6
http://hg.mozilla.org/integration/mozilla-inbound/rev/b5ae446888f5

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c2b13e9506a0
https://hg.mozilla.org/mozilla-central/rev/4ed72e80519c
https://hg.mozilla.org/mozilla-central/rev/e89419a3b021
https://hg.mozilla.org/mozilla-central/rev/b5efa2cc01f5
https://hg.mozilla.org/mozilla-central/rev/04f2152643e6
https://hg.mozilla.org/mozilla-central/rev/b5ae446888f5

Comment 35

[Brian Nicholson (:bnicholson)]

Attachment: patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: worker is run on ui thread which can slow things down, reader icon will be shown for more invalid pages, parsed article won't be available instantly
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 36

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d2e595fa3b1