sidebar.js's UpdateActiveItem handler may report unhandled promise error

RESOLVED FIXED in Firefox 40

Status

Firefox Graveyard
Reading List
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 40
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Details

Attachments

(1 attachment)

Created attachment 8586567 [details] [diff] [review]
0011-Bug-XXXXXXX-avoid-readinglist-item-races-logging-unh.patch

If you toggle the same page fast enough via the URLBar, you will eventually see the promise error below.

Full message: TypeError: item is null

is from the code:

      ReadingList.itemForURL(msg.url).then(item => {
        this.activeItem = this.itemNodesById.get(item.id);
      });

I believe this is simply a race - by the time the message gets to us the item has already been removed so I think we just need to check item (and also check the node isn't null, as that may also race if onItemDeleted already removed it)

Full error:


A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: item is null
Full stack: onMessage/<@chrome://browser/content/readinglist/sidebar.js:472:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:671:7
TaskImpl_run@resource://gre/modules/Task.jsm:319:13
Promise*ReadingListImpl.prototype._forEachRecord<@resource:///modules/readinglist/ReadingList.jsm:249:24
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
ReadingListImpl.prototype.forEachItem<@resource:///modules/readinglist/ReadingList.jsm:228:11
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
ReadingListItemIterator.prototype.items<@resource:///modules/readinglist/ReadingList.jsm:995:59
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
ReadingListImpl.prototype.item<@resource:///modules/readinglist/ReadingList.jsm:419:19
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
ReadingListImpl.prototype.itemForURL<@resource:///modules/readinglist/ReadingList.jsm:431:19
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
onMessage@chrome://browser/content/readinglist/sidebar.js:471:7
init/<@chrome://browser/content/readinglist/sidebar.js:64:49
Attachment #8586567 - Flags: review?(bmcbride)
Flags: qe-verify-
Flags: firefox-backlog+

Updated

3 years ago
Assignee: nobody → mhammond
Blocks: 1132074
Status: NEW → ASSIGNED
Iteration: --- → 40.1 - 13 Apr

Updated

3 years ago
Priority: -- → P3
Attachment #8586567 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 1

3 years ago
Comment on attachment 8586567 [details] [diff] [review]
0011-Bug-XXXXXXX-avoid-readinglist-item-races-logging-unh.patch

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

Approval Request Comment
[Feature/regressing bug #]: reading list
[User impact if declined]: Errors may be reported to the console if the user quickly adds and removes items from the reading list
[Describe test coverage new/current, TreeHerder]: Existing tests pass.
[Risks and why]: Low risk, limited to readinglist sidebar.
[String/UUID change made/needed]: None
Attachment #8586567 - Flags: approval-mozilla-beta?
Attachment #8586567 - Flags: approval-mozilla-aurora?

Comment 2

3 years ago
https://hg.mozilla.org/mozilla-central/rev/fd2dfe2228b6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8586567 [details] [diff] [review]
0011-Bug-XXXXXXX-avoid-readinglist-item-races-logging-unh.patch

Should be in 38 beta 3.
Attachment #8586567 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8586567 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fb9041fcc77
status-firefox39: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/115865f14324
status-firefox38: affected → fixed
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.