Closed Bug 1488206 Opened Last year Closed Last year

The third pane in the inspector becomes completely blank (incl. tabs) when working with 2 devtools open in separate windows.

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: Honza, Assigned: miker)

References

Details

(Whiteboard: [dt-q])

Attachments

(1 file)

From Sentry:

TelemetryStopwatch resource://devtools/client/shared/TelemetryStopwatch.jsm in timeElapsed error requesting elapsed time for nonexisting stopwatch.

  timeElapsed(histogram, object, key, aCanceledOkay) {
    const startTime = Timers.get(histogram, object, key);
    if (startTime === null) {
      if (!aCanceledOkay && !this._suppressErrors) {
        Cu.reportError("TelemetryStopwatch: requesting elapsed time for " +
                       `nonexisting stopwatch. Histogram: "${histogram}", ` +
                       `key: "${key}"`);
      }
      return -1;
    }

resource://devtools/client/shared/TelemetryStopwatch.jsm in timeElapsed at line 374:9
resource://devtools/client/shared/TelemetryStopwatch.jsm in finish at line 395:19
resource://devtools/client/shared/TelemetryStopwatch.jsm in finish at line 220:12
resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/telemetry.js in finish at line 140:12
resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/telemetry.js in toolClosed at line 638:7
self-hosted in toolClosed at line 975:17
resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/toolsidebar.js in destroy at line 449:7
self-hosted in next at line 1212:9

Sentry event:
https://sentry.prod.mozaws.net/operations/nightly-js-errors/issues/4635244/

Honza
@Mike: what could be the problem here?

Honza
Flags: needinfo?(mratcliffe)
It looks like the toolbox has been opened and closed before any tools have been initialized.

toolsidebar.js::destroy() then calls toolClosed() for the current tool, which would probably be inspector and TelemetryStopwatch.js throws.

I'll take a look.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Flags: needinfo?(mratcliffe)
Because firefox can be closed with the toolbox in a half initialized state telemetry can throw if it expects a tool to be closing when no tools are fully initialized although this is completely expected. Here we simply choose not to report those errors because this is expected behaviour.
Attachment #9006019 - Flags: review?(odvarko)
Upgrading this to P1 as another symptom of this bug is described in bug 1488835.
Priority: -- → P1
The third pane in the inspector becomes completely blank (incl. tabs) when working with 2 devtools open in separate windows.

STR:
1. Open 2 Firefox windows, open the Inspector in both of them
2. In window 1, do some interaction like switching the panel to the fonts editor
3. In window 2, try to switch the panel as well to something else

ER: Panel switching should work

AR: In window 2, the panel becomes blank
Summary: TelemetryStopwatch: error requesting elapsed time for nonexisting stopwatch → The third pane in the inspector becomes completely blank (incl. tabs) when working with 2 devtools open in separate windows.
Duplicate of this bug: 1488835
OS: Unspecified → All
Hardware: Unspecified → All
So the problem is that we currently keep the telemetry data inside a bunch of static maps inside telemetry.js.

Using static collections makes sense because multiple instances of telemetry need to access the same data so I don't think we have any choice.

Unfortunately, this causes issues when using multiple tabs or windows. Here is a simplified example:

Window 1: Opens a toolbox and sends telemetry.start("inspector")
Window 2: Opens a toolbox and sends telemetry.start("inspector"), which overwrites Window 1s start time.
Window 1: Sends telemetry.finish("inspector"), which will log a delta based on *window 2s* start time. The start time is then erased.
Window 2: Sends telemetry.finish("inspector") but there is no longer a start time so it throws an error.

Solutions:

Because devtools are tab-centric we need to find a way to connect each telemetry call with the originating tab, or even better, the toolbox inside that tab. We also need to bear in mind that the telemetry call may not be originating from the current tab.

We currently try to do this by getting the toolbox's sessionId but we only use it in a couple of circumstances. We ideally need some way to automatically discover which tab's toolbox is calling a telemetry.js method. Failing this I guess we need all telemetry calls to include a session id, which may not be possible.
An easy way to find all of our event telemetry calls is by searching for "devtools.main" (including the quotes). Al of these require a session id.

We also need to pass a unique object with start and finish calls. This needs to be the same object for calls to both start and finish.
I have decided to perform a quick workaround for now and split the more complete fix into a new bug.
Whiteboard: [dt-q]
Bug 1491879 will contain the full fix... this bug contains a temporary workaround.
Attachment #9006019 - Flags: review+ → review?(odvarko)
Comment on attachment 9006019 [details]
Bug 1488206 - TelemetryStopwatch: error requesting elapsed time for nonexisting stopwatch

Jan Honza Odvarko [:Honza] has approved the revision.
Attachment #9006019 - Flags: review+
Attachment #9006019 - Flags: review?(odvarko)
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/705f3c3d5d60
TelemetryStopwatch: error requesting elapsed time for nonexisting stopwatch r=Honza
https://hg.mozilla.org/mozilla-central/rev/705f3c3d5d60
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment on attachment 9006019 [details]
Bug 1488206 - TelemetryStopwatch: error requesting elapsed time for nonexisting stopwatch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Telemetry issue

User impact if declined: Blank panes in DevTools toolbox when opened in multiple tabs.

It is not possible to create automated tests for this particular issue.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Low risk... simple changes to a single file.

String changes made/needed:
Attachment #9006019 - Flags: approval-mozilla-release?
Mike, this temporary fix landed on Sept 20 on 64 and the real fix mentionned in comment #19 landed on Sept 27. Does this mean that the temporary fix needs to be reverted now?
Flags: needinfo?(mratcliffe)
(In reply to Pascal Chevrel:pascalc from comment #20)
> Mike, this temporary fix landed on Sept 20 on 64 and the real fix mentionned
> in comment #19 landed on Sept 27. Does this mean that the temporary fix
> needs to be reverted now?

Sorry for the misunderstanding... this (bug 1488206) is a temporary fix that would hide the problem on release (Firefox 63).
Flags: needinfo?(mratcliffe)
Comment on attachment 9006019 [details]
Bug 1488206 - TelemetryStopwatch: error requesting elapsed time for nonexisting stopwatch

Thanks for the explanations, I would probably have taken the fix in beta but not for a dot release where we want to prevent any additional risk of regressions. A temporary patch for one of our devtools in a specific workflow is not enough of a broad impact to our users to include it as a ride along in 63.0.3
Attachment #9006019 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.