Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate Dialog return value avoids mistakes reuse reference #40

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

r888800009
Copy link
Contributor

@r888800009 r888800009 commented Sep 3, 2024

inochi-creator issue
Inochi2D/inochi-creator#392
Inochi2D/inochi-creator#378

Reproduce the crash

  1. open inx
  2. save as new inx
  3. export png or video
  4. waiting autosave

upstream pr: Inochi2D/inochi-creator#393

@@ -129,7 +129,7 @@ void incClearImguiData() {
Returns the current project path
*/
string incProjectPath() {
return currProjectPath;
return currProjectPath.dup;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't see entire part of the code now, but it seems duplicating every time in incProjectPath might be heavy operation if it is used in update loop of UI. if that happens, string duplication should be called at client side and only if it should be duplicated.

Copy link
Contributor Author

@r888800009 r888800009 Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this function is only called when file access, so encapsulates and protects the internal resources of incProjectPath.

Additionally, the issue appears to be caused by incShowSaveDialog() and incShowOpenDialog(), as they may be called in other places as well. I will apply a patch there too. that only called once when the user needs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found incProjectPath is called in every update loop in incCheckAutosave.
So dup should not be called inside incProjectPath.

I still don't understand why this dup is needed yet.

Copy link
Contributor Author

@r888800009 r888800009 Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the root cause is that after users save as new .inx and export png or videos, they share the string returned by incShowSaveDialog()

leading to the two issues mentioned above.
The crash on macOS differ from those on Windows, windows is access denied, macos indicates that it is a directory.

Copy link
Contributor Author

@r888800009 r888800009 Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause the user to overwrite the incProjectPath after exported the png.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the d language cast is using the c_string memory address, I'm not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bug can also be reproduced like this

  1. open inx
  2. save as new inx
  3. export png or video
  4. Ctrl+s

You can see that the saved path is incorrect, but it will not crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incCheckAutosave seems to be called per frame, that may need to be modified

@r888800009 r888800009 changed the title Copy currProjectPath object to prevent autosave crashes Duplicate Dialog return value avoids mistakes reuse reference Sep 4, 2024
@seagetch
Copy link
Contributor

seagetch commented Sep 4, 2024

I couldn't reproduce this bug by myself. (MacOS)
I guess this is windows specific bug, with tinyfilefdialogs.

Or some sort of buffer overflow happens.
Anyway, first step seems to duplicate string which is passed as a input to tinyfiledialogs.

@r888800009
Copy link
Contributor Author

r888800009 commented Sep 4, 2024

@seagetch macos can be reproduced successfully, you may need to shorten the interval or set a hotkey invoke incAutosaveProject(incProjectPath);

std.file.FileException@std/file.d(840): /Users/user/Library/Application Support/nijigenerate/autosaves/未命名nx/nijigenerate-project.lock: Is a directory
----------------
??:? object.Throwable.TraceInfo core.runtime.defaultTraceHandler(void*) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:227 void nijigenerate.io.autosave.incCreateLockfile(immutable(char)[]) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:162 void nijigenerate.io.autosave.incAutosaveProject(immutable(char)[]) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:129 void nijigenerate.io.autosave.incCheckAutosave() [0xxxxxxxxxx]
/Users/user/nijigenerate/source/app.d:136 void app.incUpdate() [0xxxxxxxxxx]
/Users/user/nijigenerate/source/app.d:109 _Dmain [0xxxxxxxxxx]

In this case, I first "save as" and then export an "未命名" png. then call the autosave function

This bug can also be reproduced like this

  1. open inx
  2. save as new inx
  3. export png or video
  4. Ctrl+s
    You can see that the saved path is incorrect, but it will not crash. Make sure to use main branch

Make sure the two names are different The export name seems to need to be shorter than inx to reproduce

  1. open "aka.inx"
  2. save as "123456789" inx
  3. add camera (Can be skipped)
  4. export "123" png
  5. wait auto save
std.file.FileException@std/file.d(840): /Users/user/Library/Application Support/nijigenerate/autosaves/1236789/nijigenerate-project.lock: Is a directory
----------------
??:? object.Throwable.TraceInfo core.runtime.defaultTraceHandler(void*) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:227 void nijigenerate.io.autosave.incCreateLockfile(immutable(char)[]) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:162 void nijigenerate.io.autosave.incAutosaveProject(immutable(char)[]) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:129 void nijigenerate.io.autosave.incCheckAutosave() [0xxxxxxxxxx]
/Users/user/nijigenerate/source/app.d:136 void app.incUpdate() [0xxxxxxxxxx]
/Users/user/nijigenerate/source/app.d:109 _Dmain [0xxxxxxxxxx]

@r888800009
Copy link
Contributor Author

r888800009 commented Sep 4, 2024

case 2

  1. open "aka.inx"
  2. save as "ABCDEFGHIJK" inx
  3. export "1" png
  4. wait auto save
std.file.FileException@std/file.d(840): /Users/user/Library/Application Support/nijigenerate/autosaves/1DEFGHIJK/nijigenerate-project.lock: Is a directory
----------------
??:? object.Throwable.TraceInfo core.runtime.defaultTraceHandler(void*) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:227 void nijigenerate.io.autosave.incCreateLockfile(immutable(char)[]) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:162 void nijigenerate.io.autosave.incAutosaveProject(immutable(char)[]) [0xxxxxxxxxx]
/Users/user/nijigenerate/source/nijigenerate/io/autosave.d:129 void nijigenerate.io.autosave.incCheckAutosave() [0xxxxxxxxxx]
/Users/user/nijigenerate/source/app.d:136 void app.incUpdate() [0xxxxxxxxxx]
/Users/user/nijigenerate/source/app.d:109 _Dmain [0xxxxxxxxxx]

@seagetch seagetch merged commit 9fb0fd8 into nijigenerate:main Sep 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants