Closed Bug 1146358 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Reader Mode, defect)

39 Branch
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: avaida, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Flags: qe-verify+
This looks like a reading list issue, not a reader view issue.
Flags: needinfo?(mhammond)
Flags: needinfo?(florian)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(mhammond)
Flags: needinfo?(florian)
Iteration: --- → 39.3 - 30 Mar
Points: --- → 3
Flags: firefox-backlog+
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #8581802 - Flags: review?(adw)
Attached patch Patch (qref'd)Splinter Review
Attachment #8581802 - Attachment is obsolete: true
Attachment #8581802 - Flags: review?(adw)
Attachment #8581805 - Flags: review?(adw)
Blocks: 1132074
Iteration: 39.3 - 30 Mar → 39.2 - 23 Mar
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.
Attachment #8581805 - Flags: review?(adw) → review+
(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
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
https://hg.mozilla.org/mozilla-central/rev/c521d136b0f3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Duplicate of this bug: 1146572
We should probably move this logic to ReaderParent.jsm (only passing along some properties of the article object).
Flags: needinfo?(jaws)
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:
> SyncImpl.prototype._updateItemWithServerRecord@Sync.jsm:431:1
> TaskImpl_run@Task.jsm:314:40
> TaskImpl@Task.jsm:275:3
> createAsyncFunction/asyncFunction@Task.jsm:249:14
> SyncImpl.prototype._uploadNewItems@Sync.jsm:285:13
> TaskImpl_run@Task.jsm:314:40
> Handler.prototype.process@Promise-backend.js:867:23
> this.PromiseWalker.walkerLoop@Promise-backend.js: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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jaws)
(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:
> > SyncImpl.prototype._updateItemWithServerRecord@Sync.jsm:431:1

Thanks Andrei, this looks bad :)

Drew, I think we need a new bug?
Flags: needinfo?(jaws) → needinfo?(adw)
See Also: → 1149302
Flags: needinfo?(adw)
You need to log in before you can comment on or make changes to this bug.