Bug 21556 - Deleting same record twice leads to fatal software error
Summary: Deleting same record twice leads to fatal software error
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-12 01:24 UTC by David Cook
Modified: 2020-01-06 20:14 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21556: Deleting same record twice leads to fatal software error (2.02 KB, patch)
2018-10-12 02:02 UTC, David Cook
Details | Diff | Splinter Review
Bug 21556: Deleting same record twice leads to fatal software error (2.03 KB, patch)
2018-10-12 19:13 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 21556: Deleting same record twice leads to fatal software error (2.10 KB, patch)
2018-10-12 19:17 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 21556: Do not crash if a biblio is deleted twice (2.55 KB, patch)
2018-10-16 17:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21556: Do not crash if a biblio is deleted twice (2.60 KB, patch)
2018-10-17 01:20 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21556: Deleting same record twice leads to fatal software error (2.18 KB, patch)
2018-11-01 14:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21556: Do not crash if a biblio is deleted twice (2.67 KB, patch)
2018-11-01 14:28 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2018-10-12 01:24:47 UTC
If you click "Edit" then "Delete record" then "OK" when it prompts "Are you sure you want to delete this record?" then repeat the process again while the page is loading, you'll generate a fatal error.

On a Plack-enabled kohadevbox, you'll see:

Internal Server Error

If you look at /var/log/koha/kohadev/plack-error.log, you'll see:
 Can't call method "holds" on an undefined value at /home/vagrant/kohaclone/C4/Biblio.pm line 406.

On a regular dev instance, you might see in the browser window:

Can't call method "holds" on an undefined value at /path/to/git/C4/Biblio.pm line 406.
Comment 1 David Cook 2018-10-12 02:02:42 UTC
Created attachment 80442 [details] [review]
Bug 21556: Deleting same record twice leads to fatal software error

If you delete a record, confirm the deletion, and try to delete the
record and confirm it again befoe the first deletion completes, you'll
generate a fatal software error.

This patch unbinds the click handlers for the "Delete record" button
after the deletion is confirmed and just before the Javascript
processes the redirect to the next page in the process.

To test:
1) You will need a bibliographic record that is able to be deleted
2) Click "Edit", then "Delete record", then "OK"
3) While the browser is reloading, click "Edit", then "Delete record",
    then "OK
4) Observe a fatal error such as "Internal server error" or 'Can't call
method "holds" on an undefined value at
/home/vagrant/kohaclone/C4/Biblio.pm line 406.'
5) Apply the patch
6) Run "restart_all" on kohadevbox (or otherwise restart Plack if
    running Plack)
7) Navigate to another bibliographic record that is able to be deleted
8) Click "Edit", then "Delete record", then "OK"
9) While the browser is reloading, click "Edit", then "Delete record"
10) Observe that no fatal software error is generated
11) You are redirected to
http://localhost:8081/cgi-bin/koha/catalogue/search.pl (on kohadevbox)
Comment 2 Pierre-Marc Thibault 2018-10-12 19:13:11 UTC Comment hidden (obsolete)
Comment 3 Pierre-Marc Thibault 2018-10-12 19:17:42 UTC
Created attachment 80560 [details] [review]
Bug 21556: Deleting same record twice leads to fatal software error

If you delete a record, confirm the deletion, and try to delete the
record and confirm it again befoe the first deletion completes, you'll
generate a fatal software error.

This patch unbinds the click handlers for the "Delete record" button
after the deletion is confirmed and just before the Javascript
processes the redirect to the next page in the process.

To test:
1) You will need a bibliographic record that is able to be deleted
2) Click "Edit", then "Delete record", then "OK"
3) While the browser is reloading, click "Edit", then "Delete record",
    then "OK
4) Observe a fatal error such as "Internal server error" or 'Can't call
method "holds" on an undefined value at
/home/vagrant/kohaclone/C4/Biblio.pm line 406.'
5) Apply the patch
6) Run "restart_all" on kohadevbox (or otherwise restart Plack if
    running Plack)
7) Navigate to another bibliographic record that is able to be deleted
8) Click "Edit", then "Delete record", then "OK"
9) While the browser is reloading, click "Edit", then "Delete record"
10) Observe that no fatal software error is generated
11) You are redirected to
http://localhost:8081/cgi-bin/koha/catalogue/search.pl (on kohadevbox)

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Comment 4 Katrin Fischer 2018-10-14 10:22:57 UTC
Hi David, please don't forget to assign the bug to yourself!
Comment 5 David Cook 2018-10-14 23:10:29 UTC
(In reply to Katrin Fischer from comment #4)
> Hi David, please don't forget to assign the bug to yourself!

Oops! Thanks for the reminder!
Comment 6 Jonathan Druart 2018-10-16 17:43:38 UTC
(In reply to Pierre-Marc Thibault from comment #3)
> 2) Click "Edit", then "Delete record", then "OK"
> 3) While the browser is reloading, click "Edit", then "Delete record",
>     then "OK

Under plack it's impossible for my (local install) to recreate that, I am not that fast!

However I do recreate that:
- Click "Edit", then "Delete record", then "OK"
- You are redirected to the search form
- Click back
- Click "Edit", then "Delete record", then "OK"

=> Can't call method "holds" on an undefined value at /home/vagrant/kohaclone/C4/Biblio.pm line 387
Comment 7 Jonathan Druart 2018-10-16 17:54:47 UTC
Created attachment 80670 [details] [review]
Bug 21556: Do not crash if a biblio is deleted twice

To recreate:
- Go to a bibliographic detail page
- Delete it
- Go back
- Delete it again
=> Without this patch you will get a 500
 Can't call method "holds" on an undefined value at
 /home/vagrant/kohaclone/C4/Biblio.pm line 406.
=> With this patch applied it will silently redirect you to the search
form.

Note: We could/should improve the behavior and display a message, but
DelBiblio will need to be moved to Koha::Biblio->delete and other
callers adjusted
Comment 8 Jonathan Druart 2018-10-16 17:55:21 UTC
Second patch needs a signoff, maybe first patch is not necessary now (?)
Comment 9 Pierre-Marc Thibault 2018-10-16 18:09:59 UTC
I had the same issue for the second test, though I cannot tell if plack was on. I noticed that, when you click on "OK", the option to delete was still open afterwards. So, what I did is that, when "OK" appeared, instead of clicking "OK", I pressed "Enter" which does the same thing. Since you don't have to move the mouse, you can do Enter, click and Enter quickly enough to do the test.
Comment 10 David Cook 2018-10-17 00:12:13 UTC
Good points, Jonathan. It would be good to have a more graceful handling of this scenario.

I think disabling the "Delete Record" in the frontend UI after clicking it still makes sense, but I'm on board for fixing the backend too. 

Will look at testing/signing off later today...
Comment 11 Mark Tompsett 2018-10-17 01:20:29 UTC
Created attachment 80685 [details] [review]
Bug 21556: Do not crash if a biblio is deleted twice

To recreate:
- Go to a bibliographic detail page
- Delete it
- Go back
- Delete it again
=> Without this patch you will get a 500
 Can't call method "holds" on an undefined value at
 /home/vagrant/kohaclone/C4/Biblio.pm line 406.
=> With this patch applied it will silently redirect you to the search
form.

Note: We could/should improve the behavior and display a message, but
DelBiblio will need to be moved to Koha::Biblio->delete and other
callers adjusted

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 12 Mark Tompsett 2018-10-17 01:22:51 UTC
I think both patches should be used, because the javascript catches the client-side stuff and thus speed up ignoring things. However, if someone intentionally attempts to hack requests to the backend, the perl patch is necessary. This gives speed to the average user, and security from those trying to break Koha.
Comment 13 Jonathan Druart 2018-11-01 14:28:47 UTC
Created attachment 81813 [details] [review]
Bug 21556: Deleting same record twice leads to fatal software error

If you delete a record, confirm the deletion, and try to delete the
record and confirm it again befoe the first deletion completes, you'll
generate a fatal software error.

This patch unbinds the click handlers for the "Delete record" button
after the deletion is confirmed and just before the Javascript
processes the redirect to the next page in the process.

To test:
1) You will need a bibliographic record that is able to be deleted
2) Click "Edit", then "Delete record", then "OK"
3) While the browser is reloading, click "Edit", then "Delete record",
    then "OK
4) Observe a fatal error such as "Internal server error" or 'Can't call
method "holds" on an undefined value at
/home/vagrant/kohaclone/C4/Biblio.pm line 406.'
5) Apply the patch
6) Run "restart_all" on kohadevbox (or otherwise restart Plack if
    running Plack)
7) Navigate to another bibliographic record that is able to be deleted
8) Click "Edit", then "Delete record", then "OK"
9) While the browser is reloading, click "Edit", then "Delete record"
10) Observe that no fatal software error is generated
11) You are redirected to
http://localhost:8081/cgi-bin/koha/catalogue/search.pl (on kohadevbox)

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2018-11-01 14:28:53 UTC
Created attachment 81814 [details] [review]
Bug 21556: Do not crash if a biblio is deleted twice

To recreate:
- Go to a bibliographic detail page
- Delete it
- Go back
- Delete it again
=> Without this patch you will get a 500
 Can't call method "holds" on an undefined value at
 /home/vagrant/kohaclone/C4/Biblio.pm line 406.
=> With this patch applied it will silently redirect you to the search
form.

Note: We could/should improve the behavior and display a message, but
DelBiblio will need to be moved to Koha::Biblio->delete and other
callers adjusted

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Nick Clemens 2018-11-01 19:09:33 UTC
Awesome work all!

Pushed to master for 18.11
Comment 16 Martin Renvoize 2018-11-09 16:31:45 UTC
Pushed to 18.05.x for 18.05.06
Comment 17 Fridolin Somers 2018-11-28 13:59:28 UTC
Pushed to 17.11.x for 17.11.13

"You Only Live Twice"... so you may be deleted twice ;)
Comment 18 David Cook 2018-11-28 23:29:52 UTC
(In reply to Fridolin SOMERS from comment #17)
> Pushed to 17.11.x for 17.11.13
> 
> "You Only Live Twice"... so you may be deleted twice ;)

hehehe