Closed Bug 1165316 Opened 5 years ago Closed 5 years ago

TEST-UNEXPECTED-FAIL | suite/browser/test/browser/browser_bug581947.js | A promise chain failed to handle a rejection: - at file:$OBJDIR/dist/bin/components/nsLivemarkService.js:303 - Invalid livemark

Categories

(SeaMonkey :: Testing Infrastructure, defect)

defect
Not set

Tracking

(seamonkey2.37 fixed, seamonkey2.38 fixed)

RESOLVED FIXED
seamonkey2.38
Tracking Status
seamonkey2.37 --- fixed
seamonkey2.38 --- fixed

People

(Reporter: iann_bugzilla, Assigned: neil)

References

Details

Attachments

(2 files)

Running the test gives the following unexpected fail:
TEST-UNEXPECTED-FAIL | suite/browser/test/browser/browser_bug581947.js | A promise chain failed to handle a rejection:  - at file:$OBJDIR/dist/bin/components/nsLivemarkService.js:303 - Invalid livemark
Stack trace:
JS frame :: file:$OBJDIR/dist/bin/components/nsLivemarkService.js :: getLivemark/< :: line 303
JS frame :: self-hosted :: next :: line 624
JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 314
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 867
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 746
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.scheduleWalkerLoop/< :: line 688
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
This also hits other tests e.g. suite/common/tests/browser/browser_354894.js
Neil might know more about livemarks and/or promise chains...
Flags: needinfo?(neil)
I don't understand that stack trace; line 303 is not in getLivemark, it's in removeLivemark, and I don't know who might be calling removeLivemark, as there are no callers in suite/.
Flags: needinfo?(neil)
I actually have a tree with all of the getLivemark calls switched to use promises instead so maybe I should attach the patch and you can see whether it fixes the problem for you.
Oh, this must be fallout from bug 1094900 - livemarks no longer accept callbacks.
(which I didn't have in my tree so all my line numbers were wrong, sorry for the confusion)
Attached patch Possible patchSplinter Review
Most of this is just boilerplate:

PlacesUtils.livemarks.getLivemark(/* params */,
  function onCompletion(aSuccess, aLivemark) {
    if (Components.isSuccessCode(aSuccess)) {
      // code
    }
  }.bind(this)
);

becomes

PlacesUtils.livemarks.getLivemark(/* params */)
                     .then(aLivemark => {
  // code
}, () => undefined);

* Using => avoids having to use .bind(this)
* aSuccess is replaced by .then's success and failure functions
* Old callback code suppressed errors so using dummy failure function
  (would have to audit errors to see whether Cu.reportError is usable)
Attachment #8607421 - Flags: review?(iann_bugzilla)
Comment on attachment 8607418 [details] [diff] [review]
Possible patch

>+++ b/suite/common/places/treeView.js

>+      // Container may have closed since the call
Nit: Missing full stop.
Comment on attachment 8607421 [details] [diff] [review]
-w version for ease of review

r=me with nit fixed.
Attachment #8607421 - Flags: review?(iann_bugzilla) → review+
Assignee: nobody → neil
Status: NEW → ASSIGNED
Pushed comm-central changeset c182789fe426.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.38
Comment on attachment 8607418 [details] [diff] [review]
Possible patch

[Approval Request Comment]
Regression caused by (bug #): 1094900
User impact if declined: Livemarks don't work
Testing completed (on m-c, etc.): Landed on c-c
Risk to taking this patch (and alternatives if risky): Medium
String changes made by this patch: None
Attachment #8607418 - Flags: approval-comm-aurora?
Comment on attachment 8607418 [details] [diff] [review]
Possible patch

a=me
Attachment #8607418 - Flags: approval-comm-aurora? → approval-comm-aurora+
Duplicate of this bug: 992656
You need to log in before you can comment on or make changes to this bug.