*Group* buffer disaster fix

*Group* buffer disaster fix

[Hrvoje Niksic]

This patch should work.  Lloyd, Lars, please comment.


1998-10-21  Hrvoje Niksic  <hniksic@srce.hr>

	* mailcap.el (mailcap-save-binary-file): Use unwind-protect.

	* mm-decode.el (mm-display-external): Set undisplayer to mm
	buffer, not the current buffer; use unwind-protect.

--- mm-decode.el.orig	Wed Oct 21 14:32:26 1998
+++ mm-decode.el	Wed Oct 21 14:35:45 1998
@@ -220,8 +220,10 @@
 	  (buffer-disable-undo)
 	  (mm-set-buffer-file-coding-system 'no-conversion)
 	  (insert-buffer-substring cur)
-	  (funcall method)
-	  (mm-handle-set-undisplayer handle (current-buffer)))
+	  (let ((mm (current-buffer)))
+	    (unwind-protect
+		(funcall method)
+	      (mm-handle-set-undisplayer handle mm))))
       (let* ((dir (make-temp-name (expand-file-name "emm." mm-tmp-directory)))
 	     (filename (mail-content-type-get
 			(mm-handle-disposition handle) 'filename))
--- mailcap.el.orig	Wed Oct 21 16:27:16 1998
+++ mailcap.el	Wed Oct 21 16:27:30 1998
@@ -272,11 +272,12 @@
 
 (defun mailcap-save-binary-file ()
   (goto-char (point-min))
-  (let ((file (read-file-name
-	       "Filename to save as: "
-	       (or mailcap-download-directory "~/")))
-	(require-final-newline nil))
-    (write-region (point-min) (point-max) file)
+  (unwind-protect
+      (let ((file (read-file-name
+		   "Filename to save as: "
+		   (or mailcap-download-directory "~/")))
+	    (require-final-newline nil))
+	(write-region (point-min) (point-max) file))
     (kill-buffer (current-buffer))))
 
 (defun mailcap-maybe-eval ()


-- 
Hrvoje Niksic <hniksic@srce.hr> | Student at FER Zagreb, Croatia
--------------------------------+--------------------------------
Be nice to your kids.
They'll choose your nursing home.

Re: *Group* buffer disaster fix

[Lloyd Zusman]

Hrvoje Niksic <hniksic@srce.hr> writes:

> This patch should work.  Lloyd, Lars, please comment.

OOPS ... I spoke too soon in my previous message.  With this
`mailcap-save-binary-file' patch you answered my question while I was
still typing it. :)

I have no further comments on the `mm-decode.el' patch, which seems
fine to me.  However, I do have a question about the `mailcap.el'
patch:

> --- mailcap.el.orig	Wed Oct 21 16:27:16 1998
> +++ mailcap.el	Wed Oct 21 16:27:30 1998
> @@ -272,11 +272,12 @@
>  
>  (defun mailcap-save-binary-file ()
>    (goto-char (point-min))
> -  (let ((file (read-file-name
> -	       "Filename to save as: "
> -	       (or mailcap-download-directory "~/")))
> -	(require-final-newline nil))
> -    (write-region (point-min) (point-max) file)
> +  (unwind-protect
> +      (let ((file (read-file-name
> +		   "Filename to save as: "
> +		   (or mailcap-download-directory "~/")))
> +	    (require-final-newline nil))
> +	(write-region (point-min) (point-max) file))
>      (kill-buffer (current-buffer))))
>  
>  (defun mailcap-maybe-eval ()

Why do we need to do a `kill-buffer' in this routine at all when
`(mapcar 'mm-destroy-part gnus-article-mime-handles)' is already
being done within `gnus-summary-exit' and `gnus-group-exit-hook'?

Furthermore, by doing the `kill-buffer' here, would we be breaking
something (perhaps in a subtle manner) when the `mapcar' stuff gets
done later on?  

On the other hand, if it's best keep the `kill-buffer' call here, then
if this routine fails or is aborted, perhaps we would need to catch
this case and explicitly remove the offending `* mm*' buffer reference
from `gnus-article-mime-handles' *before* the `mapcar' ever gets
called ... ???

-- 
 Lloyd Zusman
 ljz@asfast.com

Re: *Group* buffer disaster fix

[Hrvoje Niksic]

Lloyd Zusman <ljz@asfast.com> writes:

> > --- mailcap.el.orig	Wed Oct 21 16:27:16 1998
> > +++ mailcap.el	Wed Oct 21 16:27:30 1998
> > @@ -272,11 +272,12 @@
> >  
> >  (defun mailcap-save-binary-file ()
> >    (goto-char (point-min))
> > -  (let ((file (read-file-name
> > -	       "Filename to save as: "
> > -	       (or mailcap-download-directory "~/")))
> > -	(require-final-newline nil))
> > -    (write-region (point-min) (point-max) file)
> > +  (unwind-protect
> > +      (let ((file (read-file-name
> > +		   "Filename to save as: "
> > +		   (or mailcap-download-directory "~/")))
> > +	    (require-final-newline nil))
> > +	(write-region (point-min) (point-max) file))
> >      (kill-buffer (current-buffer))))
> >  
> >  (defun mailcap-maybe-eval ()
> 
> Why do we need to do a `kill-buffer' in this routine at all when
> `(mapcar 'mm-destroy-part gnus-article-mime-handles)' is already
> being done within `gnus-summary-exit' and `gnus-group-exit-hook'?

Because we want the "*mm*" buffer to go away as soon as we're done
with it.  Also, the code killing the buffer was already there -- no
`+' before it.  I only added the unwind-protect bit.

-- 
Hrvoje Niksic <hniksic@srce.hr> | Student at FER Zagreb, Croatia
--------------------------------+--------------------------------
Speak softly and carry a +6 two-handed sword.

Re: *Group* buffer disaster fix

[Lloyd Zusman]

Hrvoje Niksic <hniksic@srce.hr> writes:

> [ ... ]

> > Why do we need to do a `kill-buffer' in this routine at all when
> > `(mapcar 'mm-destroy-part gnus-article-mime-handles)' is already
> > being done within `gnus-summary-exit' and `gnus-group-exit-hook'?
> 
> Because we want the "*mm*" buffer to go away as soon as we're done
> with it.  Also, the code killing the buffer was already there -- no
> `+' before it.  I only added the unwind-protect bit.

I know that you didn't add the `kill-buffer'.

I was wondering if the `mapcar' that gets invoked later might break
something, since the reference to the ` *mm*' buffer will be put into
the mime handle alist for future deletion, even though that buffer
gets killed here.

However, I just now figured out that this is probably not a problem,
since `mm-destroy-part' only does a `kill-buffer' if `buffer-live-p'
succeeds on the buffer referenced in this alist.  Hence, in this case,
we're safe to do the `kill-buffer' on the ` *mm*' buffer *and* to add
a reference to this killed buffer to the alist.

Now, as long as we're sure that no other code tries to do something to
this killed `* mm*' buffer in the alist prior to the `mapcar' call,
then we're OK.

-- 
 Lloyd Zusman
 ljz@asfast.com

Re: *Group* buffer disaster fix

[Hrvoje Niksic]

Lloyd Zusman <ljz@asfast.com> writes:

> I was wondering if the `mapcar' that gets invoked later might break
> something, since the reference to the ` *mm*' buffer will be put
> into the mime handle alist for future deletion, even though that
> buffer gets killed here.
> 
> However, I just now figured out that this is probably not a problem,
> since `mm-destroy-part' only does a `kill-buffer' if `buffer-live-p'
> succeeds on the buffer referenced in this alist.  Hence, in this
> case, we're safe to do the `kill-buffer' on the ` *mm*' buffer *and*
> to add a reference to this killed buffer to the alist.

Yup, that was exactly my reasoning.  Lars will be the final arbiter on 
this, of course.  When changing the code not all of which I
understand, I'm trying to change the least possible amount.

-- 
Hrvoje Niksic <hniksic@srce.hr> | Student at FER Zagreb, Croatia
--------------------------------+--------------------------------
HOW YOU CAN TELL THAT IT'S GOING TO BE A ROTTEN DAY:
        #15 Your pet rock snaps at you.

Re: *Group* buffer disaster fix

[Lars Magne Ingebrigtsen]

Hrvoje Niksic <hniksic@srce.hr> writes:

> 	* mailcap.el (mailcap-save-binary-file): Use unwind-protect.
> 
> 	* mm-decode.el (mm-display-external): Set undisplayer to mm
> 	buffer, not the current buffer; use unwind-protect.

Thanks for the patch; I've applied it to Pterodactyl Gnus v0.37.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: *Group* buffer disaster fix

[Lars Magne Ingebrigtsen]

Lloyd Zusman <ljz@asfast.com> writes:

> I was wondering if the `mapcar' that gets invoked later might break
> something, since the reference to the ` *mm*' buffer will be put into
> the mime handle alist for future deletion, even though that buffer
> gets killed here.

No, the " *mm*" buffer isn't killed by `mailcap-save-binary-file';
it's the "*mm*" buffer that's killed.

The latter function is also put into the handle for future automatic
destruction, but that doesn't hurt.  There is no policy for whether
"external" viewers should kill the buffer it works in after it's done
-- if the viewer deems is handy, it does, and if not, it's cleaned up
automatically later.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: *Group* buffer disaster fix

[Lloyd Zusman]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Lloyd Zusman <ljz@asfast.com> writes:
> 
> > I was wondering if the `mapcar' that gets invoked later might break
> > something, since the reference to the ` *mm*' buffer will be put into
> > the mime handle alist for future deletion, even though that buffer
> > gets killed here.
> 
> No, the " *mm*" buffer isn't killed by `mailcap-save-binary-file';
> it's the "*mm*" buffer that's killed.

I'll have to go back to the code and review the difference in usage
between the leading-space and non-leading-space versions of these
buffers.

> The latter function is also put into the handle for future automatic
> destruction, but that doesn't hurt.  There is no policy for whether
> "external" viewers should kill the buffer it works in after it's done
> -- if the viewer deems is handy, it does, and if not, it's cleaned up
> automatically later.

I therefore assume that it's guaranteed that these buffers will never
be accessed again once `(funcall method)' returns and the buffer
references are stored in the handle.  I'm relieved.

So, it looks like Hrvoje Niksic's patches involving `unwind-protect'
are indeed the proper ones to apply.  I'm curious: will these patches
be incorporated into the next pgnus release, or are you thinking of
taking a different approach to eliminate this problem?

-- 
 Lloyd Zusman
 ljz@asfast.com

Re: *Group* buffer disaster fix

[Lars Magne Ingebrigtsen]

Lloyd Zusman <ljz@asfast.com> writes:

> So, it looks like Hrvoje Niksic's patches involving `unwind-protect'
> are indeed the proper ones to apply.  I'm curious: will these patches
> be incorporated into the next pgnus release, 

Yes.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen