Closed Bug 1679043 Opened 3 years ago Closed 3 years ago

Style editor editing deletes lines and jumps around

Categories

(DevTools :: Style Editor, defect, P2)

Product:
DevTools
Component:
Style Editor
Version:
Firefox 83
Type:
defect

Tracking

(firefox86 fixed)

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox86 --- fixed

People

(Reporter: v, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Screen Recording 2020-11-23 at 17.50.36.mov
6.56 MB, video/quicktime
Details
styleeditor_doublecolon_bug.gif
150.90 KB, image/gif
Details
Bug 1679043 - [devtools] Fix race condition in StyleEditor when editing style sheet. r=daisuke,ochameau.
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

Go to Edit styles in the CSS style editor. Start editing some css.

I'm on mac os catalina, 10.15.7.

Actual results:

Text typed in disappear seemingly randomly. Sometimes the cursor also randomly jumps forward or backwards, causing you to type in the middle of a word.

Sorry that this is not very clearly described or exact, I have a hard time wording the problem but I've recorded a video of the behavior in question.

I have not been able to figure out exactly what triggers it. It happens quite often though and is extremely frustrating when it does.

Expected results:

css should just be typed in as in any normal editor.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Style Editor
Product: Firefox → DevTools

Can reproduce:
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:84.0) Gecko/20100101 Firefox/84.0

Running Firefox Developer Edition version 84.0b3 (64-bit)

macOS Catalina, 10.15.4

Looks similar to Bug 1673302.

Thanks for the video, it helps although I can't reproduce this locally.

I have not been able to figure out exactly what triggers it. It happens quite often though and is extremely frustrating when it does.

Any chance you have a website publicly available where this happens quite often?
Also would you be able to describe how frequently it happens when you use the styleeditor? Once every 10 seconds? Once every 10 minutes? Just to get an idea of how we should try to replicate the bug.

Flags: needinfo?(viktor.holmberg)
See Also: → 1673302

It has happened quite often when working on my work website: https://www.medino.com or my personal site https://viktor.pictures .

It is quite tricky to say how often it happens, seems that it gets into some state and then it happens constantly, like every time I type a word in. I have not actually checked if a restart solves it. But once it's fine, it's fine, If that makes sense - it's just when it gets into the "mess up all text" mode that it happens all the time.
How often it gets into the state is hard to say, now it hasn't happened for a few days. So like 2 times a week it gets into the bad state.

I appreciate it's hard to reproduce. Is there anything I can test or capture to help repo next time it goes into the bad state?

Flags: needinfo?(viktor.holmberg) → needinfo?(jdescottes)

Thanks, this info is really helpful!
I am not using the styleeditor for long sessions, so it can explain why I didn't experience this so far.

We recently updated the version of codemirror used in DevTools from 5.49.0 to 5.58.1.
This was done in Bug 1668480, which landed in Firefox 83. All the reports we had about this are from FF83 so it could be linked.
If we find a scenario to reproduce this, it would be interesting to test if downgrading codemirror fixes the issue.
Note that FF ESR is on v78, so it still uses the old codemirror in case you want to try this.

Also noting that all the reports until now are on MacOS Catalina, but it might not be specific to this OS.

I'm going to assign it P2 priority, since we've had 2 reports about this. Keeping S3 severity since this doesn't fully prevent using the browser or DevTools (even if it's really annoying, totally understand that!)

Is there anything I can test or capture to help repo next time it goes into the bad state?

You could open the Browser Console (shift+command+J) and check if there are errors logged there. They could relate to something that went wrong in DevTools.

Severity: -- → S3
Flags: needinfo?(jdescottes)
Priority: -- → P2
See Also: → 1668480

(In reply to Julian Descottes [:jdescottes] from comment #5)

Thanks, this info is really helpful!
I am not using the styleeditor for long sessions, so it can explain why I didn't experience this so far.

We recently updated the version of codemirror used in DevTools from 5.49.0 to 5.58.1.
This was done in Bug 1668480, which landed in Firefox 83. All the reports we had about this are from FF83 so it could be linked.
If we find a scenario to reproduce this, it would be interesting to test if downgrading codemirror fixes the issue.
Note that FF ESR is on v78, so it still uses the old codemirror in case you want to try this.

I misread, the other bug was open for FF82, so before the codemirror upgrade.

Confirmed exact same behaviour for me:
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:83.0) Gecko/20100101 Firefox/83.0
Windows 10 v2004, build 19041.630

So not just MacOS. I noticed the problem started a couple of months ago but I don't know which version.

This week I tried Developer Edition (latest version) as a brand new install with no add-ons, etc. and could reproduce the problem. I then uninstalled Dev Edition and installed v80 which works fine.

I can't see any pattern for when the problem occurs and it happens with different sites. It generally happens almost straight away when I'm editing CSS.

Hope this helps? I'll try a couple of different versions over the next few days and try and work out which ones are affected.

Seems to happen especially with autocomplete and selectors like ::after, trying to replicate this: (the comment is autocomplete)

.someDiv {...}
/*    | ::hover | */
.someDiv::hov

.someDiv {...}
.someDiv::hov::hover

(In reply to nutdriver716 from comment #8)

Seems to happen especially with autocomplete and selectors like ::after, trying to replicate this: (the comment is autocomplete)

Thanks, I can see a reproducible issue with ::. I don't think I get exactly the same symptoms as you. I can't get duplicated :: after validating the autocompletion. But it's still broken. Using a single : doesn't seem to trip the autocomplete.

I will file a separate bug for this, because it feels different from the issue described in the summary.
Edit: Filed Bug 1679953

See Also: → 1679953

This bug has been happening to me for a couple of versions now. Style editor works just fine but then all of a sudden, starts to mess up. Most common glitch is if there's white-space (indentation with tabs) at the beginning of the line, completing an style attribute will cause the white-space to disappear. Other glitches include just typing and suddenly the last several (and not a consistent amount of) characters disappear. Only way to fix is close Firefox completely and start over. Closing the debugger does fix it temporarily but the bug returns much sooner than closing Firefox altogether. My guess is it has something to do with the code formatting/validation.

This has been happening to me and drives me crazy.

Seems to only happen for local files, but I can't be 100% sure. It's almost like the style editor refreshes mid-edit and removes properties because they're invalid - of course they are, I'm in the middle of typing them.

Again, can't be sure, but seems like what's happening to me.

So far, I still didn't manage to reproduce the issue.

Seems to only happen for local files,

Thanks for the suggestion, tried with a local project, but still no bug.

Closing the debugger does fix it temporarily but the bug returns much sooner than closing Firefox altogether.

Starting to wonder if this is hardware (memory?) related. If this is somehow linked to a memory leak that could explain why closing Firefox is the only way to actually fix the issue. Would be helpful to know what kind of hardware people experiencing issues are using?

My main machine has 32gb of ram but I should have access to an older one with only 8gb of ram (just tried a quick 5mn session with it, no bug yet)

In my case it happens with files on a remote server - I haven't tried local files.

For me it feels like the last few keystrokes are deleted/undone. If I copy and paste a chunk of CSS it is not deleted, although the position shifts (as if I've scrolled the window).

I'm not sure that is it memory related. It happened to me a moment ago and when I switched to a different tab it was fine. Then switched back to the original tab and it was still misbehaving. But FWIW I have 16GB.

I'm struggling to find any kind of pattern. Sometimes it happens and other times it doesn't. When it happens it happens for all stylesheets for the current site so that would suggest it's not something specific to certain stylesheets.

This bug sent me to the DevTools community again! I have the same issue adding a new stylesheet to the Style Editor. I do that very heavily because I style my new websites that way, and then copy-paste the styles to my project stylesheet. My project has a lot of CSS imports, etc, I don't know what's exactly causing the issue but I'll try to solve it on my end, hopefully, that will solve it for everyone.

My steps to reproduce:
Go to https://ctvt.staging.coffee, open the Style Editor, create a new sheet, work as usual. Cursor wraps, things delete.

I'm going to be downloading the source tonight, hopefully, I can find some clues.

I can also confirm the bug exists; I'm using Windows 10 with Firefox 83.0 (64-bit). However, a reason you may have a hard time reproducing the bug is because it, at least for me, is not browser-wide. It only seems to affect certain websites and not others. I don't know if this has to do with scripts loaded by the pages or some combination of things. I am trying to get a list of sites it doesn't work on then see if there is a common thread.

I decided to look at the devtools style-editor code a bit to try to figure out what might be causing this. To me it looks like the problem is essentially this:

  1. After each modification the text in the editor is being run through _getSourceTextAndPrettify() in onStyleApplied - StyleSheetEditor.jsm
  2. _getSourceTextAndPrettify() itself calls prettifyCSS() and when it returns the text in the editor will be replaced
  3. but there could be changes between when _getSourceTextAndPrettify() was called and when it returned.

I can build Firefox and remove the commands to replace the editor content (lines 402-404) with the prettified version and that seems to get rid of the issue completely as far as I can tell. However, it seems a bit confusing why prettifyCSS doesn't result in consistently bad behavior.

Nonetheless, I'm not sure why we would even want to prettify the full text while doing manual modifications - that sounds rather nonsensical but maybe I'm not considering something here.

For the record, the problem (at least for me) is 100% reproducible while editing userChrome.css via browser-toolbox - however I am able to reproduce it on normal web-pages albeit less consistently.

Thanks for the investigation jastekken!
I managed to reproduce once, but then I got a crash before I could fully debug the issue

(In reply to jastekken from comment #16)

Nonetheless, I'm not sure why we would even want to prettify the full text while doing manual modifications - that sounds rather nonsensical but maybe I'm not considering something here.

You're totally right, and this is why we have this _isUpdating property to ignore changes applied from the style editor (https://searchfox.org/mozilla-central/rev/23c25cd32a1e87095301273937b4ee162f41e860/devtools/client/styleeditor/StyleSheetEditor.jsm#392-395)
So it looks like, at some point, we might be missing that.
Would you be able to check if we're having a race condition or something?

Here we're simply managing a flag, which I guess can be brittle:

  1. user type a letter
  2. isUpdating is set to true and we call the server to apply the changes
  3. user type another letter
  4. isUpdating is set to true again, and we call the server to apply the changes a second time
  5. we receive the first event that indicates a changes was made. isUpdating is still true, so we enter the if, and as a result, set isUpdating to false
  6. we receive the second event that indicates a changes was made. isUpdating is now false, and we get into the else branch, which we shouldn't

repeat those steps with multiple keypresses, and we have a pretty racy behaviour. This probably is hard to reproduce as we wait for 500ms (https://searchfox.org/mozilla-central/rev/23c25cd32a1e87095301273937b4ee162f41e860/devtools/client/styleeditor/StyleSheetEditor.jsm#32) before calling the server (or if another keystroke comes, we cancel the call, and wait for 500ms again).

One way to fix this would be to change the usage of isUpdating from a simple boolean to a counter (e.g. pendingUpdates), that we would increment or decrement.

Flags: needinfo?(jastekken)

jastekken , since you can reproduce quite easily, would you have the time to look into this?

Yeah, I can look into this when I have time.

However, what I meant earlier asking why we prettify the text on each edit still confuses me. I mean, is it really necessary to do that? Why not just prettify when the stylesheet was loaded into the editor once? If we did that then this race condition (or whatever) wouldn't have any visible side-effects as far as I can tell.

Regardless, if anyone can figure out a web page where this is anywhere near consistent then I would appreciate that. I seem to stumble upon this on my normal build with local files occasionally but the problem doesn't necessarily arise when I load those in my build profile.

Flags: needinfo?(jastekken)

(In reply to jastekken from comment #19)

However, what I meant earlier asking why we prettify the text on each edit still confuses me. I mean, is it really necessary to do that? Why not just prettify when the stylesheet was loaded into the editor once? If we did that then this race condition (or whatever) wouldn't have any visible side-effects as far as I can tell.

So I don't think the bug comes from the fact that we prettify, it just that we set the text of the editor each time some styles are applied.
This behavior was added in Bug 984880. It was a way to reflect in the StyleEditor any changes made to the stylesheet (it might be from a script for example, or from the inspector). The fact that we prettify after editing, meaning _isUpdating is false, is the real bug.

I'm also going to take a look see if I can cleanup the code, even if I'm not reproducing the issue easily.

Assignee: nobody → nchevobbe
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

I pushed to TRY a WIP patch https://treeherder.mozilla.org/jobs?repo=try&revision=ded260d67d1cfd2527d1227d444390e9e9189048
People who are experiencing this issue, would you download the binary, install it and test if that fixes the issue for you?

Binaries are available for a given platform by clicking on the green Ba text, and then selecting Artifact in the bottom panel that opens.

I'll build tonight and let you know Nicolas.

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #21)

I pushed to TRY a WIP patch https://treeherder.mozilla.org/jobs?repo=try&revision=ded260d67d1cfd2527d1227d444390e9e9189048
People who are experiencing this issue, would you download the binary, install it and test if that fixes the issue for you?

Binaries are available for a given platform by clicking on the green Ba text, and then selecting Artifact in the bottom panel that opens.

Great, I cannot reproduce the issue with this build.

It's hard to say for certain of course because of the inconsistent nature of the issue but I am pretty confident since it was reproducible ~100% of the time using browser toolbox but now I cannot seem to reproduce it at all.

okay that sounds great. I'll cleanup that patch and publish it for review

When the user edits a stylesheet in the StyleEditor, an _isUpdating flag is toggled,
and a request is sent to the server to actually apply those changes to the stylesheet.
It then causes a style-applied event to be emitted (or the stylesheet resource
to be updated, if watcher support is enabled for stylesheet).
In the end, this triggers the onStyleApplied function in the StyleEditor, where
we check if the _isUpdating flag is true (to know if the event was caused
through editing in style editor), and if not, replace the stylesheet content.

Unfortunately there's a race condition when the user is typing (and sending
multiple requests to the server), as the state of the _isUpdating flag could
be wrong if a new request is sent before the first one is handled.
This is probably highlighted with the throttling we're doing in the Resource API.

To fix this issue, we add a new cause parameter to the StyleSheets.update method,
which we set to styleeditor when calling update within the StyleEditor.
This cause parameter is then sent back by the server to the client (via the
style-applied event, or the resource update if we have Watcher support for stylesheets).
This cause can be checked by the StyleEditor client, and replace the check
on _isUpdating.

However, we need to keep the _isUpdating property to handle backward compatibility.

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/682d68dbcb46
[devtools] Fix race condition in StyleEditor when editing style sheet. r=daisuke,ochameau,devtools-backward-compat-reviewers,bomsy.

https://hg.mozilla.org/mozilla-central/rev/682d68dbcb46

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox86: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

Can confirm that I does not seem to happen anymore on nightly. Super amazed with how fast you guys managed to fix it, I did not expect that :nchevobbe or :jastekken! This is why firefox is the best :D

I still have a problem with the style editor where undoing or redoing (especially going back more than like 10-20 actions) sometimes causes the entire stylesheet text to disappear. It comes back if I click on it, but I can't see what I'm undoing/redoing, and when it comes back it's usually scrolled to the top of the sheet. It only seems to happen in the browser toolbox, not the content toolbox, but idk for sure since I use the browser toolbox way more often. It also seems to happen way more often if I switch between the style editor tab and inspector and console tabs, etc. If I'm only using the style editor tab and haven't loaded any other dev tool tabs then this usually doesn't happen at all.

The error still remains and I can confirm that the described behaviour of aminomancer also exists on my system.

amonimancer, benk, what version are you using? The issue was fixed in 86, which is now in Beta, and will be in release in ~ a week

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #34)

amonimancer, benk, what version are you using? The issue was fixed in 86, which is now in Beta, and will be in release in ~ a week

Hi, I was using FF 85.02. That was my mistake. In the build 86.0b9 everything works fine for me. Thank you

87.0a1, build ID 20210216094005.
the original problem described in this bug report has been fixed for a while now, i was describing something different in my comment above. but maybe with a similar etiology since they both involve things somehow going haywire when using the undo function. but this bug where the page goes blank happens a lot more rarely and less reliably. the other bug was something that just happened all the time no matter what, at least for me. i noticed both around the same time so naturally i thought this bug fix would also fix the undo problem. just didn't get around to posting about it until now

(In reply to aminomancer from comment #36)

87.0a1, build ID 20210216094005.
the original problem described in this bug report has been fixed for a while now, i was describing something different in my comment above. but maybe with a similar etiology since they both involve things somehow going haywire when using the undo function. but this bug where the page goes blank happens a lot more rarely and less reliably. the other bug was something that just happened all the time no matter what, at least for me. i noticed both around the same time so naturally i thought this bug fix would also fix the undo problem. just didn't get around to posting about it until now

Thanks for the info, let's file a separate bug.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: