1146358 - The attempt of adding a page to Reading List from Reader View fails and causes an error

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Andrei Vaida [:avaida]]

Reproducible on:
Nightly 39.0a1 (2015-03-23)

Affected platforms:
Windows 7 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5

Preconditions:
* browser.readinglist.enabled = true (default)
* reader.parse-on-load.enabled = true (default)

Steps to reproduce:
1. Launch Firefox.
2. Go to: (e.g.) https://www.mozilla.org/en-US/mission/.
3. From the Location Bar, click the "Enter Reader View" button.
4. From Reader View's controls, click the "Add page to Reading List" button.

Expected result:
The page is successfully added to Reading List, without causing any issues or throwing any errors.

Actual result:
Clicking the "Add page to Reading List" button does nothing and throws the following error in the Browser Console:
> A promise chain failed to handle a rejection. 
> Did you forget to '.catch', or did you forget to 'return'?
> See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
> 
> Date: Mon Mar 23 2015 14:16:37 GMT+0200 (EET)
> Full Message: Error: Unrecognized item property: byline
> Full Stack: normalizeRecord@resource:///modules/readinglist/ReadingList.jsm:857:1
> ReadingListImpl.prototype.addItem<@resource:///modules/readinglist/ReadingList.jsm:227:14
> TaskImpl_run@resource://gre/modules/Task.jsm:314:40
> TaskImpl@resource://gre/modules/Task.jsm:275:3
> createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
> ReaderParent.receiveMessage@resource:///modules/ReaderParent.jsm:46:9
> ReadingList.jsm:857:0

Note:
* This seems to be a recent regression. I'll follow up with a regression range as soon as possible.

Comment 1

[:Margaret Leibovic]

This looks like a reading list issue, not a reader view issue.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Margaret pointed out to me that Android does similar things to filter out unused properties from Readability, http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Reader.js#253

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (qref'd)

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8581805 [details] [diff] [review]
Patch (qref'd)

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

OK, but I wonder if it might be better for the consumer to do this instead of making ReadingList have to know about these properties that otherwise don't have anything to do with it.

::: browser/components/readinglist/ReadingList.jsm
@@ +61,5 @@
> +// Article objects that are passed to ReadingList.addItem may contain
> +// some properties that are known but are not currently stored in the
> +// ReadingList records. This is the list of properties that are knowingly
> +// disregarded before the item is normalized.
> +const ITEM_DISREGARDED_PROPERTIES = ["byline", "dir", "content", "length"];

Would you mind listing these like the other similar lists in this file?

const ITEM_DISREGARDED_PROPERTIES = `
  byline
  dir
  ...
`.trim().split(/\s+/);

@@ +859,5 @@
>  function normalizeRecord(nonNormalizedRecord) {
>    let record = {};
>    for (let prop in nonNormalizedRecord) {
> +    if (ITEM_DISREGARDED_PROPERTIES.includes(prop)) {
> +      delete nonNormalizedRecord[prop];

I don't think you need to delete the prop, so could you please remove this?  normalizeRecord is called in two places, and in both places the original object isn't used afterward, so there's no danger of accidentally using a disregarded property.  I want to avoid modifying the objects that the consumer passes in.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/c521d136b0f3

Comment 6

[:Margaret Leibovic]

(In reply to Drew Willcoxon :adw from comment #4)

> OK, but I wonder if it might be better for the consumer to do this instead
> of making ReadingList have to know about these properties that otherwise
> don't have anything to do with it.

In the case of Android, we do this filtering in Reader.js (our equivalent of ReaderParent.jsm), before we send the reading list items over to our Java reading list service.

An equivalent place to do this on desktop would be in the "Reader:AddToList" message handler in ReaderParent.jsm:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#45

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

We should probably move this logic to ReaderParent.jsm (only passing along some properties of the article object).

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/61685c383230

Comment 11

[Andrei Vaida [:avaida]]

An item can now be successfully added to Reading List *from* Reader View, but the following error is now being thrown by Log.jsm:74 only on Windows 7 (x64) with Aurora 38.0a2 (2015-03-29): 
> 1427696235770 readinglist.scheduler ERROR Sync failed, now in state 'other error': Error: Item should exist > (resource:///modules/readinglist/Sync.jsm:431) JS Stack trace:
> [e-mail]:431:1
> [e-mail]:314:40
> [e-mail]:275:3
> createAsyncFunction/[e-mail]:249:14
> [e-mail]:285:13
> [e-mail]:314:40
> [e-mail]:867:23
> [e-mail]:746:7
> this.PromiseWalker.scheduleWalkerLoop/@Promise-backend.js:688:37 Log.jsm:74

Jared: I don't think this error is related to this bug, but perhaps we could clarify that here. Either way, I'm gonna mark this fix verified and file a separate bug for the new error, if that's necessary.

Testing covered Nightly 39.0a1 (2015-03-29) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Andrei Vaida, QA [:avaida] from comment #11)
> An item can now be successfully added to Reading List *from* Reader View,
> but the following error is now being thrown by Log.jsm:74 only on Windows 7
> (x64) with Aurora 38.0a2 (2015-03-29): 
> > 1427696235770 readinglist.scheduler ERROR Sync failed, now in state 'other error': Error: Item should exist > (resource:///modules/readinglist/Sync.jsm:431) JS Stack trace:
> > [e-mail]:431:1

Thanks Andrei, this looks bad :)

Drew, I think we need a new bug?