Potential crashes between source deletion and accessing source in the gui #906

sssoleileraaa · 2020-03-11T01:47:16Z

Description

If a sync comes back with deleted sources from the server we delete them locally and then call update_sources which will first remove all the source widgets then remove all the conversation widgets for those sources. If a message, reply, or file download job finishes before the source widget is destroyed then it will try to access source.uuid or source.collection in several places in the code and crash the app. I think there is also an issue with a user trying to open a file for the source at this time.

To prevent this type of crash we should only access the source db object when absolutely necessary. For instance, it's not necessary to store source on an object when we only need the uuid so we can find a source widget.

The text was updated successfully, but these errors were encountered:

sssoleileraaa · 2020-03-19T20:51:15Z

Here's a list of areas in the code that need attention regarding possible crashes due to trying to access data on a deleted source without catching the exception:

✔️DONE in delete source widgets for current sync update #1114
SourceWidget.update raises exception and callers don't catch it causing the app to crash
- add-source scheduler (STR in add-source scheduler crashes app because of unhandled exception #1117)
- in the constructor`:
  
  securedrop-client/securedrop_client/gui/widgets.py
  
  Line 1175 in f1e935c
  
  self.update()
✔️DONE in Fix starring behavior #952
StarToggleButton
DeleteSourceMessageBox:

securedrop-client/securedrop_client/gui/widgets.py

Line 1414 in f1e935c

for submission in source.collection:
ConversationView:

securedrop-client/securedrop_client/gui/widgets.py

Line 3005 in f1e935c

self.update_conversation(self.source.collection)
on_reply_sent:

securedrop-client/securedrop_client/gui/widgets.py

Line 3149 in f1e935c

if source_uuid == self.source.uuid:
SourceConversationWrapper:

securedrop-client/securedrop_client/gui/widgets.py

Line 3174 in f1e935c

self.source_uuid = source.uuid
on_source_change:

securedrop-client/securedrop_client/gui/widgets.py

Line 735 in f1e935c

self.source_conversations[source.uuid] = conversation_wrapper
✔️DONE in delete source widgets for current sync update #1114
- uuid in SourceLists.update:
  
  securedrop-client/securedrop_client/gui/widgets.py
  
  Line 932 in f1e935c
  
  source_uuids = [source.uuid for source in sources]
- last_updated in SourceLists.update when source widgets are reordered (STR in client crashes when sources are reordered #1116)
DeleteSourceMessageBox:

securedrop-client/securedrop_client/gui/widgets.py

Line 1368 in 78d1bbe

for submission in source.collection:
ReplyBox:

securedrop-client/securedrop_client/gui/widgets.py

Line 3271 in 78d1bbe

if self.source.public_key:
SourceProfileShortWidget:

securedrop-client/securedrop_client/gui/widgets.py

Line 3650 in f1e935c

self.updated.setText(_(arrow.get(self.source.last_updated).format('DD MMM')))
✔️DONE in no longer access source db obj in snippet #964
set_snippet
✔️DONE in delete source widgets for current sync update #1114
schedule_source_management:

securedrop-client/securedrop_client/gui/widgets.py

Line 1005 in 97ce5bb

self.source_widgets[source.uuid] = new_source

Some of these links take you to areas of the code where there are multiple instances of the issue, so look around for source or file or other database objects widgets are accessing to make sure you get em all. Also I might have missed some in the list above. If you want to fix the issue for now by surrounding the code in a try-catch, that works, but it's more ideal if you can remove the db objects from the widgets.

sssoleileraaa · 2020-03-20T16:55:25Z

Numbers: 1, 2, and 8 have PRs. The rest are still up for grabs.

sssoleileraaa · 2020-03-26T23:04:56Z

Saw # 1 crasher in the list happen today:

Traceback (most recent call last):
  File "/home/creviera/workspace/freedomofpress/securedrop-client/securedrop_client/gui/widgets.py", line 1004, in schedule_source_management
    new_source = SourceWidget(self.controller, source)
  File "/home/creviera/workspace/freedomofpress/securedrop-client/securedrop_client/gui/widgets.py", line 1220, in __init__
    self.update()
  File "/home/creviera/workspace/freedomofpress/securedrop-client/securedrop_client/gui/widgets.py", line 1227, in update
    self.controller.session.refresh(self.source)
  File "/home/creviera/workspace/freedomofpress/securedrop-client/.venv/lib/python3.7/site-packages/sqlalchemy/orm/session.py", line 1675, in refresh
    "Could not refresh instance '%s'" % instance_str(instance)
sqlalchemy.exc.InvalidRequestError: Could not refresh instance '<Source at 0x7f04fd010790>'

eloquence · 2020-03-27T21:43:24Z

We discussed today that a naive try/catch strategy around all UI access to widgets that may be tied to deleted database objects is probably the best first iteration to safeguard this. We'll likely not get to this before Monday, but it's one of the highest priority known areas that can cause crashes, so tracking as release blocker.

eloquence · 2020-04-02T21:24:56Z

Implementing the try/catch strategy mentioned above is a goal for the 4/2-4/15 sprint; the more comprehensive approach in #967 can be revisited at a later time.

sssoleileraaa · 2020-04-02T21:50:04Z

(hit this again today after deleting some sources server-side and starting the client: #906 (comment))

eloquence · 2020-04-22T22:46:38Z

Deferring to near-term backlog as it's not been a significant issue for pilot installs so far. We'll revisit this in the next sprint (after the current 4/22-5/6 sprint); given that we do know that there's crash potential here, I do think it should go into 0.2.1/0.3.0 at the latest.

sssoleileraaa · 2020-05-05T18:03:09Z

We saw during the pilot that a new piece of code that was added after this issue was opened caused a segfault. The reason is that we access last_updated on the source db object without making sure it's valid:

securedrop-client/securedrop_client/gui/widgets.py

Lines 907 to 908 in 5055040

    
           my_ts = arrow.get(me.source.last_updated) 
        
           other_ts = arrow.get(them.source.last_updated)

This can be added as part of # 8 in the list above.

eloquence · 2020-06-03T20:48:03Z

For the 6/3-6/17 sprint, @creviera has agreed to take a time-boxed stab at this to make an initial round of stability improvements, aiming for 8 hours (that should ideally include review, so I recommend making the implementation time-box shorter).

sssoleileraaa · 2020-06-23T19:52:14Z

Update on the progress of this issue:

What's been fixed?

1. ✔️DONE in #1114
SourceWidget.update raises exception and callers don't catch it causing the app to crash

add-source scheduler (STR in add-source scheduler crashes app because of unhandled exception #1117)
in the constructor:

securedrop-client/securedrop_client/gui/widgets.py

Line 1175 in f1e935c

self.update()

2. ✔️DONE in #952
StarToggleButton

8. ✔️DONE in #1114

uuid in SourceLists.update:

securedrop-client/securedrop_client/gui/widgets.py

Line 932 in f1e935c

source_uuids = [source.uuid for source in sources]
last_updated in SourceLists.update when source widgets are reordered (STR in client crashes when sources are reordered #1116)

12. ✔️DONE in #964
set_snippet

13. ✔️DONE in #1114
schedule_source_management:

securedrop-client/securedrop_client/gui/widgets.py

Line 1005 in 97ce5bb

self.source_widgets[source.uuid] = new_source

What's left?

3. DeleteSourceMessageBox:

securedrop-client/securedrop_client/gui/widgets.py

Line 1414 in f1e935c

for submission in source.collection:

4. ConversationView:

securedrop-client/securedrop_client/gui/widgets.py

Line 3005 in f1e935c

self.update_conversation(self.source.collection)

5. on_reply_sent:

securedrop-client/securedrop_client/gui/widgets.py

Line 3149 in f1e935c

if source_uuid == self.source.uuid:

6. SourceConversationWrapper:

securedrop-client/securedrop_client/gui/widgets.py

Line 3174 in f1e935c

self.source_uuid = source.uuid

7. on_source_change:

securedrop-client/securedrop_client/gui/widgets.py

Line 735 in f1e935c

self.source_conversations[source.uuid] = conversation_wrapper

9. DeleteSourceMessageBox:

securedrop-client/securedrop_client/gui/widgets.py

Line 1368 in 78d1bbe

for submission in source.collection:

10. ReplyBox:

securedrop-client/securedrop_client/gui/widgets.py

Line 3271 in 78d1bbe

if self.source.public_key:

11. SourceProfileShortWidget:

securedrop-client/securedrop_client/gui/widgets.py

Line 3650 in f1e935c

self.updated.setText(_(arrow.get(self.source.last_updated).format('DD MMM')))

eloquence · 2020-06-24T18:21:24Z

Thanks @creviera for the writeup. Since you've addressed the issues which were straightforward to reproduce and similar to reports we've heard from prod users, we've agreed to defer additional work on this for now, but will pick this issue up again and leave it open for tracking purposes.

zenmonkeykstop · 2024-08-15T18:53:39Z

Closing, looks like it was addressed. May reopen as part of a future sync/data model milestone.

eloquence changed the title ~~Potentional crashes between source deletion and accessing source in the gui~~ Potential crashes between source deletion and accessing source in the gui Mar 11, 2020

This was referenced Mar 16, 2020

use the id of the widget, not the db object #943

Merged

accessing the source db object when starring after deletion crashes client #946

Closed

Crash on source selection #899

Closed

Crash on source selection during on_source_change #947

Closed

This was referenced Mar 19, 2020

no longer access source db obj in snippet #964

Merged

Remove source db object from SourceWidget #967

Closed

sssoleileraaa added the bug Something isn't working label Mar 24, 2020

sssoleileraaa mentioned this issue Mar 24, 2020

Updates the filesize after download and decryption #969

Merged

6 tasks

eloquence added the release blocker label Mar 27, 2020

eloquence removed the release blocker label Mar 30, 2020

eloquence added this to the 0.2.0 milestone Apr 1, 2020

eloquence modified the milestones: 0.2.0, 0.3.0 May 5, 2020

eloquence mentioned this issue Jun 9, 2020

New source never transitions out of "Awaiting key" state #862

Closed

This was referenced Jun 16, 2020

delete source widgets for current sync update #1114

Merged

client crashes when sources are reordered #1116

Closed

add-source scheduler crashes app because of unhandled exception #1117

Closed

eloquence removed this from the 0.3.0 milestone Jun 24, 2020

sssoleileraaa mentioned this issue Jul 19, 2021

Add safe deletion parity with journalist interface #1263

Merged

10 tasks

zenmonkeykstop added the sync sync issues for future epic label Aug 15, 2024

zenmonkeykstop closed this as completed Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Potential crashes between source deletion and accessing source in the gui #906

Potential crashes between source deletion and accessing source in the gui #906

sssoleileraaa commented Mar 11, 2020

sssoleileraaa commented Mar 19, 2020 •

edited

Loading

sssoleileraaa commented Mar 20, 2020

sssoleileraaa commented Mar 26, 2020 •

edited

Loading

eloquence commented Mar 27, 2020 •

edited

Loading

eloquence commented Apr 2, 2020

sssoleileraaa commented Apr 2, 2020

eloquence commented Apr 22, 2020

sssoleileraaa commented May 5, 2020

eloquence commented Jun 3, 2020

sssoleileraaa commented Jun 23, 2020

eloquence commented Jun 24, 2020

zenmonkeykstop commented Aug 15, 2024

Potential crashes between source deletion and accessing source in the gui #906

Potential crashes between source deletion and accessing source in the gui #906

Comments

sssoleileraaa commented Mar 11, 2020

Description

sssoleileraaa commented Mar 19, 2020 • edited Loading

sssoleileraaa commented Mar 20, 2020

sssoleileraaa commented Mar 26, 2020 • edited Loading

eloquence commented Mar 27, 2020 • edited Loading

eloquence commented Apr 2, 2020

sssoleileraaa commented Apr 2, 2020

eloquence commented Apr 22, 2020

sssoleileraaa commented May 5, 2020

eloquence commented Jun 3, 2020

sssoleileraaa commented Jun 23, 2020

What's been fixed?

What's left?

eloquence commented Jun 24, 2020

zenmonkeykstop commented Aug 15, 2024

sssoleileraaa commented Mar 19, 2020 •

edited

Loading

sssoleileraaa commented Mar 26, 2020 •

edited

Loading

eloquence commented Mar 27, 2020 •

edited

Loading