Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Get a console error if you open a project using "Application Support" or its parent folder. #7300

Closed
RaymondLim opened this issue Mar 23, 2014 · 18 comments
Assignees
Milestone

Comments

@RaymondLim
Copy link
Contributor

  1. Drag your "Application Support" folder from Finder to Brackets to use it as a new project.
  2. From project drop down list switch to the previous project.
  3. Open Developer Tools from Debug menu.
  4. Look in the Console Tab or click on the error icon in the status bar.

Result: You will see one console error.

Blind write attempted:  /Users/rlim/Library/Application Support/Brackets/state.json 1395536064000 null AppshellFileSystem.js:426

Although we allow blind write in sprint 36, we're not updating state.json in this case. (or maybe an out-of-order overwrite is taking out all the newly updates).

If you restart or reload Brackets, you will see the project you opened in step 1 as current project, not the one you switched in step 2 even if it is the default Getting Started project (issue #7299). Note that you can also reproduce it with Brackets cache folder.

@RaymondLim
Copy link
Contributor Author

Add step 2 that is necessary for the console error.

This issue was discovered when helping @lkcampbell's to identify the cause of #7299. I was able to reproduce #7299 when dragging the entire user folder as the project root, but not with "Application Support" folder.

@peterflynn
Copy link
Member

I'm almost positive the problem is the File entry for state.json being held onto after one of its ancestor folders undergoes an unwatch() (more details in my note in #7299). We should probably close one or the other of these as a dupe.

@peterflynn
Copy link
Member

Also, seems unlikely to be Mac-only... just easier to hit on Mac since opening ~ will hit this case, whereas on Win you'd have to open a weirder folder to hit it.

@peterflynn peterflynn added this to the Release #38 milestone Mar 24, 2014
@peterflynn
Copy link
Member

Hmm, interestingly though FileStorage.save() & load() fetch a new File object every time... so either someone else is holding onto that entry, or something else funny is going on...

@RaymondLim
Copy link
Contributor Author

Remove mac only label since I can reproduce it on Windows also.

@peterflynn
Copy link
Member

Ok, I figured out what's going on:

  1. We initially read state.json very early in startup (when the preferences modules first load). This sets the File entry's hash.
  2. Later, we open the initial project. (Which triggers a no-op write to state.json, but that's irrelevant here).
    Now, here's where things diverge...
  3. We switch to a different project - the old project root is unwatched
    • In the normal case, this means nothing special
    • In the bug case, since state.json was actually in the old project root, its File entry is killed off - FileSystem basically forgets it's ever seen that file before
  4. We try to write the new 'last project' setting to state.json
    • In the normal case, this succeeds since we still have the hash from step 1
    • In the bug case, we create a brand-new, never-read File entry. Writing fails since FileSystem thinks we didn't read the file before trying to write it (hash is still undefined).

So, I think this is all normal & correct FileSystem behavior -- by design, you're not supposed to be able to write to a file if we don't have a guarantee that you've read it before. That constraint doesn't really make sense for the preferences code. So I think we should fix this by having PreferencesBase pass the blind option to write(), disabling that check.

We could instead try to do something clever to retain the old hash, or re-read state.json on each project load... but really, we probably want the hash check disabled all the time for prefs, not just in this one specific case. E.g. imagine some other app touches that file for some reason... then you keep using Brackets for a while. You wouldn't want all your view-state changes after the external-touch to be dropped on the floor. We really do want to overwrite the file in all cases, regardless of what its timestamp on disk says (which is how the old localStorage prefs worked too).

CC @iwehrman @gruehle @dangoor in case you guys might have different opinions. If not I can put up a PR with the simple fix tomorrow.

@peterflynn
Copy link
Member

Btw @dangoor can you think of any reasonably simple way to unit-test the fix for this? Now that the prefs unit tests all use in-memory storage it seems tricky.

@gruehle
Copy link
Member

gruehle commented Mar 28, 2014

As long as the state.json contents are not intended to be user-edited, I think it's fine to do a blind write. If the file is meant to be edited, or if the write code is shared with prefs.json, then we could have a situation where user-edited changes get clobbered. Admittedly, this is an extreme edge case, but it should be considered.

@peterflynn
Copy link
Member

Hmm, the prefs.json case is trickier. It would be subject to this exact same bug, but it is intended to be user-editable. It seems not unreasonable to favor changes made in the editor UI -- i.e. use the blind flag (simple fix proposed above) and know that we risk overwriting hand-editing changes if file watchers have failed for some reason. The only other option would be to do something hacky like explicitly re-read the file on each project load (or maybe just when unloading a project that was a parent of the prefs folder)...

@dangoor Any thoughts here? Personally I'm leaning toward the simple fix, given that we think we've gotten watchers to be pretty reliable at this point...

@peterflynn
Copy link
Member

Actually, I just realized the preferences system doesn't use file watchers -- prefs.json is usually outside the watch root, and the preferences system doesn't add a second watch root for it, so we only pick up prefs changes made using Brackets as the editor (because the prefs code listens for documentSaved events). Any external changes are ignored until Brackets is restarted, and with my proposed fix about those external changes would usually get overwritten and lost. I guess we do need to do something fancier here afterall...

@dangoor
Copy link
Contributor

dangoor commented Mar 31, 2014

@peterflynn Yeah, I agree that we'd want to prefer the changes that the user makes and just write to the file, but only as long as we're watching the file for external changes.

@peterflynn
Copy link
Member

Hrm, actually I forgot a critical point in all this: up until now we've never exercised the FileSystem code with multiple "watch roots" active at once. In theory it all works, but I'd be leery of starting to rely on it now without a ton of testing runway. And there's one further wrinkle: it doesn’t currently allow overlapping roots, so we'd have to unwatch the prefs file whenever opening a project root that includes that file in its subtree, and rewatch the prefs file whenever we close such a project.

So I think for now it may actually be cleaner to do something hacky, like forcibly re-read the prefs file from disk whenever we switch away from a project that contained it...

@dangoor
Copy link
Contributor

dangoor commented Mar 31, 2014

Yes, I think you're right that a simpler solution (even if it feels a little hacky) is a better choice right now.

peterflynn added a commit that referenced this issue Apr 1, 2014
contains the user prefs JSON files in its subtree) - ensure that FileSystem
always knows we've read the JSON file before we try to write to it. We
don't want to enable the 'blind' flag since it will mean we virtually
always overwrite external changes to the prefs file, and we don't want to
start using file watchers to observe external changes on the fly since
we haven't yet deeply tested having multiple watch roots active at once.
- Rename confusing "filename" vars in PreferencesBase
- Fix JSLint errors in ProjectManager from #7026
@peterflynn
Copy link
Member

I've spun off #7375 for using file watchers to get nicer behavior

peterflynn added a commit that referenced this issue Apr 2, 2014
Fix bug #7300 (Prefs never saved again after opening ancestor folder of user prefs.json)
@peterflynn
Copy link
Member

Oops, forgot to reassign - FBNC @RaymondLim

@RaymondLim
Copy link
Contributor Author

Confirmed fixed.

@lkcampbell
Copy link
Contributor

Just an FYI, confirmed that issue #7299 is fixed as well. Thanks.

@peterflynn
Copy link
Member

@lkcampbell Thanks, good to have the confirmation!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants