Closed Bug 1354800 Opened 3 years ago Closed 3 years ago

TelemetryStopwatch: key "FX_TAB_CLOSE_TIME_ANIM_MS" was already initialized TelemetryStopwatch.jsm:299

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: dao, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(1 file)

(In reply to tabmix.onemen from bug 1340842 comment 43)
> This patch causing TelemetryStopwatch error when underflow call
> tabbrowser.removeTab for all tabbrowser._removingTabs
> 
> TelemetryStopwatch: key "FX_TAB_CLOSE_TIME_ANIM_MS" was already initialized
> TelemetryStopwatch.jsm:299
> 	start resource://gre/modules/TelemetryStopwatch.jsm:299:7
> 	start resource://gre/modules/TelemetryStopwatch.jsm:136:12
> 	removeTab chrome://browser/content/tabbrowser.xml:2549:13
> 	onxblunderflow chrome://browser/content/tabbrowser.xml:5631:11
> 	permitUnload chrome://global/content/bindings/remote-browser.xml:357:13
> 	_beginRemoveTab chrome://browser/content/tabbrowser.xml:2645:46
> 	removeTab chrome://browser/content/tabbrowser.xml:2570:18
> 	removeTabsToTheEndFrom chrome://browser/content/tabbrowser.xml:2490:17
> 	oncommand
Flags: needinfo?(mconley)
Blocks: 1340842
Flags: needinfo?(mconley)
Comment on attachment 8857097 [details]
Bug 1354800 - Account for re-entry to removeTab function for tab close timing probes.

https://reviewboard.mozilla.org/r/129004/#review131586

::: browser/base/content/tabbrowser.xml:2556
(Diff revision 1)
> +            // not we're allowed to unload them via permitUnload. This means that
> +            // subsequent calls to removeTab will also have these stopwatches
> +            // on file. There's also the case that the tab was animating closed,
> +            // and removeTab was called on it before it completed. In either case,
> +            // we skip (re)starting the stopwatches.
> +            if (!aTab._pendingPermitUnload && !aTab.closing) {

Not a huge fan of these super-verbose comments. It's kind of burying the code in the middle of an essay that isn't even relevant to most people looking at removeTab. I feel that this should be briefer, and if people need more detail they can hunt down the commit / bug.
Attachment #8857097 - Flags: review?(dao+bmo) → review+
Comment on attachment 8857097 [details]
Bug 1354800 - Account for re-entry to removeTab function for tab close timing probes.

https://reviewboard.mozilla.org/r/129004/#review131586

> Not a huge fan of these super-verbose comments. It's kind of burying the code in the middle of an essay that isn't even relevant to most people looking at removeTab. I feel that this should be briefer, and if people need more detail they can hunt down the commit / bug.

Fair point. I'll try to tone it down in future patches.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/971334a063db
Account for re-entry to removeTab function for tab close timing probes. r=dao
https://hg.mozilla.org/mozilla-central/rev/971334a063db
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee: nobody → mconley
You need to log in before you can comment on or make changes to this bug.