Page MenuHome

Fix T88570: Crash when saving after pressing ctrl+S twice.
ClosedPublic

Authored by Julian Eisel (Severin) on Dec 1 2021, 6:53 PM.

Details

Summary

Existing code to replace the file operation was failing when done from the
window for the file operation itself.

Basically, this patch does two things:

  • More carefully select the window to use as the "root" or parent of the File Browser window.
  • Because of that, the context also needs to be set more carefully, to be valid for that window.

Idea is to more carefully set a "root context" (as in, the context from where
the File Browser was started from) with clearly defined rules. It is set up by
WM_event_add_fileselect() and passed to the file-select handler. On
execution of the file operation, it is restored (if possible), so the context of
the operation matches the context where the File Browser was invoked in.
When reusing the File Browser window, we also reuse the same root
area/region in the new file-select handler, so these are well defined for the
operator execution (and not dependent on the current mouse position for
example). Previously it would just hope the current context works well
enough for that.

Alternative to D12592.

NOTE: I would not suggest applying this for the 3.0 release. Changes here are quite risky. Plenty of high priority bugs were in this code in the past.

Diff Detail

Repository
rB Blender

Event Timeline

Julian Eisel (Severin) requested review of this revision.Dec 1 2021, 6:53 PM
Julian Eisel (Severin) created this revision.
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Dec 1 2021, 7:21 PM

I found a crash if you

  • Open another temp window (e.g. Render Window)
  • swap the image viewer area for a filebrowser
  • Ctrl+S
  • Split the area
  • Ctrl+S with the cursor over the newly created area

Causes an access violation in rna_pointer_inherit_refine. Also found some non-crashing, but still weird behavior like pictured:

Own comment in D12592:

Editing of temp screens should be disabled, see rB87aca8bd02bc. Unfortunately that commit didn't nearly cover all cases.

There's also code already to prevent other edits to temp screens, like changing the workspace or screen. I would suggest we treat that as a separate issue.

I'd say this case only worked by chance earlier, because there happened to be a valid context.

source/blender/windowmanager/intern/wm_event_system.c
2531

A further improvement could be splitting the file-select handler off from the regular operator handler. Then this could read fileselect_handler->root_context.win.

2614

Want to do this separately.

  • Merge branch 'master' into temp-fix-T88570

Testing this patch and still getting a crash.

  • Open Blender with the factory settings.
  • Press Ctrl-S.
  • N (to show the side-bar).
  • Hold Ctrl-S, while doing so move the mouse between the root window and the file-selector.

Blender crashes P2930.

Crashes, although it this seems like it might not be directly related to this report so it could be resolved separately.


This seems like a fairly complex solution, while it makes sense it would be nice if simpler logic could be used.
Is there really a need to have multiple file selectors is open at once (with different root windows). It seems like this could be simplified by only supporting a single file-selector open with an operator assigned.

Otherwise this patch seems OK.

source/blender/editors/space_file/filesel.c
1351–1353

Since this patch was created, doc-strings are now added to the headers.

1371

Could be called ED_fileselect_handler_area_find_any_with_op (since this needs an operator).

Submitted fix for crash noted in last comment. D14905

Accepting as using a root window seems necessary which for file selector functionality to work properly and I don't see a way to do this in a simpler way. In my tests to completely resolve this crash the SpaceFile.op needs to be cleared as well (see D14905 for reference).

This revision is now accepted and ready to land.May 10 2022, 10:25 AM
Julian Eisel (Severin) marked 2 inline comments as done.
  • Address review requests
  • Make comments more clear

This seems like a fairly complex solution, while it makes sense it would be nice if simpler logic could be used.

Agreed the complexity is annoying. But it's good to have a well defined state now, this was just lacking completely.

Thanks for checking so thoroughly and the extra fix!