1118806 - Handle uncaught promise rejections in the storage inspector

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect)

Description

[Eddy Bruel [:ejpbruel]]

Attempting to replace the deprecated-sync-thenables with Promise.jsm promises in protocol.js (bug 1096490) uncovered some uncaught promise rejections in the inspector. These need to be fixed before I can land the former.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Handle uncaught promise rejections

I don't know who owns the storage inspector so flagging pbrosset for review is a guess.

Patrick, if you're not the right person for this feel free to unassign yourself. Or even better, assign the right person instead :-)

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8545312 [details] [diff] [review]
Handle uncaught promise rejections

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

Simple enough change for R+ even if I don't own this panel.

Comment 3

[Eddy Bruel [:ejpbruel]]

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-dt orange.
https://hg.mozilla.org/integration/fx-team/rev/8d3e410aa24a

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1637841&repo=fx-team

Comment 5

[Eddy Bruel [:ejpbruel]]

For the next attempt, let's try this on try first:
https://tbpl.mozilla.org/?tree=Try&rev=c6db05fe0011

Comment 6

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/246718a1cded

Comment 7

[Eddy Bruel [:ejpbruel]]

Try push was green, but discovered another uncaught rejection, so here's a second push just to make sure:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=39880ae71804

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Handle uncaught promise rejections

Try push looks green. Rerequesting review for the additional changes.

Apparently I accidentally squashed two commits, so there are also some style inspector changes in this patch. It's less than 3 lines though, so easiest would be to just land it as one patch.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8548018 [details] [diff] [review]
Handle uncaught promise rejections

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

r=me with a comment added

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +183,5 @@
>    _hideCurrent: function() {
>      if (this.highlighterShown) {
>        this._getHighlighter(this.highlighterShown).then(highlighter => {
> +        let promise = highlighter.hide();
> +        if (promise) {

I think a comment is required before this condition. It will be otherwise hard to future readers to understand why hide sometimes returns a promise and sometimes not.

Comment 10

[Eddy Bruel [:ejpbruel]]

Here goes nothing. Nobody tell RyanVM where I live:
https://hg.mozilla.org/integration/fx-team/rev/c9ff49636eef

Comment 11

[Carsten Book [:Tomcat]]

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