Editor is occasionally empty

RESOLVED FIXED

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
a month ago
20 days ago

People

(Reporter: jlast, Assigned: jlast)

Tracking

unspecified
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a month ago
We have a race condition, which has been fixed on nightly for the past month:

https://github.com/devtools-html/debugger.html/commit/c54f6544a2eb00513912817a1522773a69ff6901

This patch backports the fix to beta.
(Assignee)

Comment 1

a month ago
Created attachment 8919001 [details] [diff] [review]
patch-fix-editor-1.patch
Attachment #8919001 - Flags: review?(jlaster)
(Assignee)

Updated

a month ago
Assignee: nobody → jlaster
(Assignee)

Comment 2

a month ago
Created attachment 8919002 [details] [diff] [review]
patch-fix-editor-1-1.patch

Approval Request Comment

[Feature/Bug causing the regression]: It was introduced in an earlier release. I'm not sure which.

[User impact if declined]: The debugger editor will occasionally be blank. This will require users to close and re-open devtools.

[Is this code covered by automated tests?]: Yes.
 
[Has the fix been verified in Nightly?]: Yes. We landed the fix on nightly as part of an earlier bundle a month ago.

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: No.

[Why is the change risky/not risky?]: It is not that risky because the fix landed earlier on nightly. And we have not had any issues.

[String changes made/needed]: None
Attachment #8919001 - Attachment is obsolete: true
Attachment #8919001 - Flags: review?(jlaster)
Attachment #8919002 - Flags: review?(jdescottes)
Attachment #8919002 - Flags: approval-mozilla-beta?
> [Feature/Bug causing the regression]: It was introduced in an earlier release. I'm not sure which.
Earlier = 55 ? 56 or earlier?
Flags: needinfo?(jlaster)
Comment on attachment 8919002 [details] [diff] [review]
patch-fix-editor-1-1.patch

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

So if I summarize the changes here, we are:
- guarding a few methods with `if (!this.state.editor) return`
- "removing" the in-scope lines highlight feature (at least part of it?)
- refactoring parts of componentWillReceiveProps to setSize and setText
- applying setSize before setText

If the list above is what you were aiming for, then the code looks fine. 
Waiting for more explanations about the bug or STRs so I can verify this correctly.
Comment on attachment 8919002 [details] [diff] [review]
patch-fix-editor-1-1.patch

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

As discussed, can we have a smaller patch focused on fixing the bug?
Attachment #8919002 - Flags: review?(jdescottes) → review-
(In reply to Sylvestre Ledru [:sylvestre] from comment #3)
> > [Feature/Bug causing the regression]: It was introduced in an earlier release. I'm not sure which.
> Earlier = 55 ? 56 or earlier?

The new debugger is enabled for release users only since 56, so it's either 56 or 57.
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox58: --- → fixed
status-firefox-esr52: --- → unaffected
Comment on attachment 8919002 [details] [diff] [review]
patch-fix-editor-1-1.patch

r- on review, so, a-
we need that asap if we want to fix that in 57.
Attachment #8919002 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(Assignee)

Comment 8

a month ago
Created attachment 8920175 [details] [diff] [review]
empty-1.patch
Attachment #8919002 - Attachment is obsolete: true
Flags: needinfo?(jlaster)
Attachment #8920175 - Flags: review?(jdescottes)
Attachment #8920175 - Flags: approval-mozilla-beta?
(Assignee)

Comment 9

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4edb70c885043512b3b2d4e799724784d540475
Comment on attachment 8920175 [details] [diff] [review]
empty-1.patch

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

New version looks good to me. Tested the patch on beta, no issue spotted. 
Did not manage to reproduce the race condition on my machine.
Attachment #8920175 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 11

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2cf53a527dd538777f54a6520d53526a41e7092
ni ritu for this uplift request (beta-only patch, the bug was fixed elsewhere for trunk), see comment 2 (which hopefully still applies to the new, simpler patch).
Flags: needinfo?(rkothari)
Comment on attachment 8920175 [details] [diff] [review]
empty-1.patch

Recent regression, low risk, stabilized on Nightly for a while, Beta57+
Flags: needinfo?(rkothari)
Attachment #8920175 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: NEW → RESOLVED
Last Resolved: 23 days ago
Resolution: --- → FIXED

Comment 14

23 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/64aa5b5122f9
status-firefox57: affected → fixed
(In reply to Jason Laster [:jlast] from comment #2)
> [Is this code covered by automated tests?]: Yes.
>  
> [Has the fix been verified in Nightly?]: Yes. We landed the fix on nightly
> as part of an earlier bundle a month ago.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Jason's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.