[PATCH] emacs: show: lazy part bugfix

[PATCH] emacs: show: lazy part bugfix

[Mark Walters]

The lazy part handling had a subtle bug. Notmuch stores the part
information as a text property with the displayed part so attachment
handling (saving viewing etc work).

Now, some mime parts have subparts and to avoid overwriting the
sub-part data notmuch checks and if part data is already recorded it
does not overwrite it.

Now with lazy part handling this could fail: there is already part
data stored. In the common case it works as the part type information
was stored when the lazy-part button was inserted. However, this fails
if the lazy part has sub-parts: notmuch had no idea these existed
until the lazy part insertion.

We fix this by removing any existing part-information from the
relevant region before doing the lazy insertion.
---
This was shown up by Istvan's patch in id:m3r4dtgz9k.fsf@zsu.kismala.com and 
the bug found by Jamie in id:87fvu4fl25.fsf@servo.finestructure.net

I think this is essentially the right patch: I am not certain about
the +1 in the removing the property. It seems to be needed but maybe
something with front/back sticky would be better.

Also this definitely needs more testing before going into master: this
code is definitely fragile.

Best wishes 

Mark

 emacs/notmuch-show.el |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 20844f0..a31b6dc 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -853,6 +853,9 @@ message at DEPTH in the current thread."
   ;; character of the button by adding a newline and finish by
   ;; removing the extra newline from the end of the part.
   (save-excursion
+    ;; Remove part-information from lazy part-region
+    (put-text-property (button-start button) (1+ (button-end button)) :notmuch-part nil)
+
     (goto-char (button-end button))
     (insert "\n")
     (let* ((inhibit-read-only t)
-- 
1.7.9.1

Re: [PATCH] emacs: show: lazy part bugfix

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]

On Fri, Aug 23 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> The lazy part handling had a subtle bug. Notmuch stores the part
> information as a text property with the displayed part so attachment
> handling (saving viewing etc work).
>
> Now, some mime parts have subparts and to avoid overwriting the
> sub-part data notmuch checks and if part data is already recorded it
> does not overwrite it.
>
> Now with lazy part handling this could fail: there is already part
> data stored. In the common case it works as the part type information
> was stored when the lazy-part button was inserted. However, this fails
> if the lazy part has sub-parts: notmuch had no idea these existed
> until the lazy part insertion.
>
> We fix this by removing any existing part-information from the
> relevant region before doing the lazy insertion.
> ---
> This was shown up by Istvan's patch in id:m3r4dtgz9k.fsf@zsu.kismala.com and 
> the bug found by Jamie in id:87fvu4fl25.fsf@servo.finestructure.net
>
> I think this is essentially the right patch: I am not certain about
> the +1 in the removing the property. It seems to be needed but maybe
> something with front/back sticky would be better.
>
> Also this definitely needs more testing before going into master: this
> code is definitely fragile.

Hey, Mark.  I have just tested this patch and it seems to be fixing the
issue I was seeing.  With Istvan's patch the once hidden parts are now
exposed with buttons, and with this patch the buttons now work as
expected.

I can't really speak to the correctness of this patch, but it does seem
to be working, and doesn't seem to have any adverse side effects that I
can see.

As a reminder, this patch and Istvan's do fix an actual bug in notmuch,
so both should be pushed unless anyone see's anything wrong with either
of them.

Thanks so much for the fixes, guys.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

[PATCH v2] emacs: show: lazy part bugfix

[Mark Walters]

The lazy part handling had a subtle bug. Notmuch stores the part
information as a text property with the displayed part so attachment
handling (saving viewing etc work).

Now, some mime parts have subparts and to avoid overwriting the
sub-part data notmuch checks and if part data is already recorded it
does not overwrite it.

Now with lazy part handling this could fail: there is already part
data stored. In the common case it works as the part type information
was stored when the lazy-part button was inserted. However, this fails
if the lazy part has sub-parts: notmuch had no idea these existed
until the lazy part insertion.

We fix this by removing any existing part-information from the
relevant region before doing the lazy insertion.
---
This is the same as the previous patch
id:1377246875-7784-1-git-send-email-markwalters1009@gmail.com except I
have added a substantial comment. I now believe the +1 in the remove
part information is genuinely correct for the reason detailed in the comment.
 
This does seem to fix an actual bug (see parent message and links there).

Best wishes

Mark


 emacs/notmuch-show.el |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 20844f0..58ef4df 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -852,7 +852,20 @@ message at DEPTH in the current thread."
   ;; from a message header etc) so instead we start from the last
   ;; character of the button by adding a newline and finish by
   ;; removing the extra newline from the end of the part.
+  ;;
+  ;; Essentially, this function wants text properties to be
+  ;; front-nonsticky (where most of the other functions need
+  ;; front-sticky so that is what they actually are) and this is a way
+  ;; of faking the front-nonsticky for this one function.
+  ;;
+  ;; For exactly this reason we also remove existing part-information
+  ;; from one extra character: this extra character is the newline
+  ;; that we delete at the end.
+
   (save-excursion
+    ;; Remove part-information from lazy part-region
+    (put-text-property (button-start button) (1+ (button-end button)) :notmuch-part nil)
+
     (goto-char (button-end button))
     (insert "\n")
     (let* ((inhibit-read-only t)
-- 
1.7.9.1

Re: [PATCH v2] emacs: show: lazy part bugfix

[Austin Clements]

Quoth Mark Walters on Sep 04 at  8:30 am:
> The lazy part handling had a subtle bug. Notmuch stores the part
> information as a text property with the displayed part so attachment
> handling (saving viewing etc work).

s/ work)/) work/

> 
> Now, some mime parts have subparts and to avoid overwriting the
> sub-part data notmuch checks and if part data is already recorded it
> does not overwrite it.
> 
> Now with lazy part handling this could fail: there is already part
> data stored. In the common case it works as the part type information
> was stored when the lazy-part button was inserted. However, this fails
> if the lazy part has sub-parts: notmuch had no idea these existed
> until the lazy part insertion.

This says that things fail when a lazy part has sub-parts, but not
what the failure is.  What is the failure?  Can you give a specific
sequence of events and conditions that leads to and demonstrates the
failure?

(I ask not just for commit posterity, but because I actually don't
know, though I may have figured it out after writing the comment
below.)

> We fix this by removing any existing part-information from the
> relevant region before doing the lazy insertion.
> ---
> This is the same as the previous patch
> id:1377246875-7784-1-git-send-email-markwalters1009@gmail.com except I
> have added a substantial comment. I now believe the +1 in the remove
> part information is genuinely correct for the reason detailed in the comment.
>  
> This does seem to fix an actual bug (see parent message and links there).
> 
> Best wishes
> 
> Mark
> 
> 
>  emacs/notmuch-show.el |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 20844f0..58ef4df 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -852,7 +852,20 @@ message at DEPTH in the current thread."
>    ;; from a message header etc) so instead we start from the last
>    ;; character of the button by adding a newline and finish by
>    ;; removing the extra newline from the end of the part.
> +  ;;
> +  ;; Essentially, this function wants text properties to be
> +  ;; front-nonsticky (where most of the other functions need
> +  ;; front-sticky so that is what they actually are) and this is a way
> +  ;; of faking the front-nonsticky for this one function.

Most properties are rear-sticky and front-nonsticky.  Maybe this is
referring specifically to :notmuch-part, which is the other way
around?

> +  ;;
> +  ;; For exactly this reason we also remove existing part-information
> +  ;; from one extra character: this extra character is the newline
> +  ;; that we delete at the end.

This comment doesn't say what the root of the problem is.  Is the
following accurate?

;; First, eliminate the :notmuch-part property from the region we'll
;; be inserting this part in, since we never override an existing
;; :notmuch-part property.  If we're inserting a leaf part, it will
;; re-apply the same :notmuch-part, but if this isn't a leaf, the
;; children need to apply different :notmuch-part values.
;; :notmuch-part was originally applied to the newline following the
;; button as well, so remove it from that character, too.  (If we
;; didn't remove it from the newline, this would all be for naught:
;; :notmuch-part is front-sticky and we insert the part immediately
;; before this newline, so the part would inherit :notmuch-part from
;; the newline.)

> +
>    (save-excursion
> +    ;; Remove part-information from lazy part-region
> +    (put-text-property (button-start button) (1+ (button-end button)) :notmuch-part nil)
> +
>      (goto-char (button-end button))
>      (insert "\n")
>      (let* ((inhibit-read-only t)

Re: [PATCH v2] emacs: show: lazy part bugfix

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

On Wed, Sep 04 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> Now, some mime parts have subparts and to avoid overwriting the
>> sub-part data notmuch checks and if part data is already recorded it
>> does not overwrite it.
>> 
>> Now with lazy part handling this could fail: there is already part
>> data stored. In the common case it works as the part type information
>> was stored when the lazy-part button was inserted. However, this fails
>> if the lazy part has sub-parts: notmuch had no idea these existed
>> until the lazy part insertion.
>
> This says that things fail when a lazy part has sub-parts, but not
> what the failure is.  What is the failure?  Can you give a specific
> sequence of events and conditions that leads to and demonstrates the
> failure?
>
> (I ask not just for commit posterity, but because I actually don't
> know, though I may have figured it out after writing the comment
> below.)

Hey, Austin.  Here's an example of a mail that is effected the issue:

└┬╴multipart/alternative 896783 bytes
 ├─╴text/plain 379 bytes
 └┬╴multipart/related 892556 bytes
  ├─╴text/html 1236 bytes
  └─╴image/jpeg inline [photo.JPG] 890841 bytes

The multipart/related part is initially hidden.  Without Istvan's patch,
there would be no button at all for the image/jpeg part, even when the
multipart/related is exposed.  With Istvan's patch the image/jpeg button
is there, but without Mark's patch the button would actually reference
the entire multipart/alternative part, instead of just the image/jpeg.
If I tried to save the image/jpeg I would get the entire
multipart/alternative mime structure in plain text.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2] emacs: show: lazy part bugfix

[Austin Clements]

Quoth Jameson Graef Rollins on Sep 04 at  8:50 am:
> On Wed, Sep 04 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> >> Now, some mime parts have subparts and to avoid overwriting the
> >> sub-part data notmuch checks and if part data is already recorded it
> >> does not overwrite it.
> >> 
> >> Now with lazy part handling this could fail: there is already part
> >> data stored. In the common case it works as the part type information
> >> was stored when the lazy-part button was inserted. However, this fails
> >> if the lazy part has sub-parts: notmuch had no idea these existed
> >> until the lazy part insertion.
> >
> > This says that things fail when a lazy part has sub-parts, but not
> > what the failure is.  What is the failure?  Can you give a specific
> > sequence of events and conditions that leads to and demonstrates the
> > failure?
> >
> > (I ask not just for commit posterity, but because I actually don't
> > know, though I may have figured it out after writing the comment
> > below.)
> 
> Hey, Austin.  Here's an example of a mail that is effected the issue:
> 
> └┬╴multipart/alternative 896783 bytes
>  ├─╴text/plain 379 bytes
>  └┬╴multipart/related 892556 bytes
>   ├─╴text/html 1236 bytes
>   └─╴image/jpeg inline [photo.JPG] 890841 bytes
> 
> The multipart/related part is initially hidden.  Without Istvan's patch,
> there would be no button at all for the image/jpeg part, even when the
> multipart/related is exposed.  With Istvan's patch the image/jpeg button
> is there, but without Mark's patch the button would actually reference
> the entire multipart/alternative part, instead of just the image/jpeg.
> If I tried to save the image/jpeg I would get the entire
> multipart/alternative mime structure in plain text.

Ah.  Thanks for the example!  After staring at the patch for long
enough, I figured it had to be something like this, but still didn't
have anything concrete.  It would be great to include this in the
commit message to show exactly what was broken and what's being fixed.

Now that I'm sure what the problem was, the code itself LGTM.

Re: [PATCH v2] emacs: show: lazy part bugfix

[Mark Walters]

Thanks to Austin's prodding and debugging and pointing out contradictory
evidence I have looked into this bug in more detail and think I have got
the actual cause this time.

Having chased it all down I think there is a one line fix. Since I had
already written the rest of the email when I discovered this I include
the diagnosis below but most people will probably want to skip to the
patch at the end.

---

The problem only occurs for message of this sort of form
   └┬╴multipart/alternative 896783 bytes
    ├─╴text/plain 379 bytes
    └┬╴multipart/related 892556 bytes
     ├─╴text/html 1236 bytes
     └─╴image/jpeg inline [photo.JPG] 890841 bytes

The important features are that initially the multipart/related is
hidden, and that when the multipart/related part is shown the child
image/jpeg is hidden (that is what happens in Istvan's patch and could
also happen with the inner part also being multipart/alternative but
not, I think, if the inner part were multipart/mixed).

Now the code for hiding parts starts with the normal button and then
toggles the button (this is to make sure the button gets the correct
label text). When doing so this code looks at the first character of the
button, stores the text-properties, replaces the button with the toggled
button, puts the text-properties back. Since insert does not put in text
properties we do need this save restore.

In the normal case where a hidden part is inserted it is on a newline
where the current text-properties are nil. Thus the save/restore of text
properties does nothing in this case; after this is done the correct
text-properties are applied to the whole part (with the proviso that
:notmuch-part does not over-ride subpart's :notmuch-part property)

In the problem case above we have a line with 

[multipart/related (hidden)]

The whole of this line including the \n has the multipart-related
:notmuch-part

Now when we do the lazy insertion we toggle this button (which behaves
correctly), go to the end of the button insert a \n and then insert the
child parts.

At this point we have

[multipart/related]\n
\n

the [multipart/related] has the multipart/related :notmuch-part info the
first \n does not (as we just inserted) and the second \n does because
it is the original \n

Now if we insert a part button for a hidden part we insert the button
and toggle it. Finally we get the problem: this toggle saves and
restores (to the whole button) the text-properties from *point* which is
the \n so includes the multipart/related
:notmuch-part text-property

If we save and restore from the start of the button we solve all of the
problems (I think)

Best wishes 

Mark


---
 emacs/notmuch-show.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 20844f0..0267574 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -503,7 +503,7 @@ message at DEPTH in the current thread."
 	     (new-start (button-start button))
 	     (button-label (button-get button :base-label))
 	     (old-point (point))
-	     (properties (text-properties-at (point)))
+	     (properties (text-properties-at (button-start button)))
 	     (inhibit-read-only t))
 	;; Toggle the button itself.
 	(button-put button :notmuch-part-hidden (not show))
-- 
1.7.9.1

[PATCH] emacs: show: lazy part handling bugfix

[Mark Walters]

The lazy part handler had a bug that it allowed the button to be
toggled to be specified. During toggling it needs to save and restore
the text-properties for the button but it actually saved the text
properties at point rather than from the button.

In almost all cases this didn't matter as as point had the same text
properties as the button. However, it is a bug and did cause incorrect
behaviour in some cases: see id:87txhz14z6.fsf@qmul.ac.uk for details.
---
This is exactly the same as the patch in the parent except it has a
commit message.

Best wishes

Mark


 emacs/notmuch-show.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 20844f0..0267574 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -503,7 +503,7 @@ message at DEPTH in the current thread."
 	     (new-start (button-start button))
 	     (button-label (button-get button :base-label))
 	     (old-point (point))
-	     (properties (text-properties-at (point)))
+	     (properties (text-properties-at (button-start button)))
 	     (inhibit-read-only t))
 	;; Toggle the button itself.
 	(button-put button :notmuch-part-hidden (not show))
-- 
1.7.9.1

Re: [PATCH] emacs: show: lazy part handling bugfix

[Austin Clements]

Nice sleuthing!  LGTM.

One question, though.  `notmuch-show-insert-bodypart' calls
`notmuch-show-toggle-part-invisibility' with point at the end of the
buffer and not on the button.  Without this patch, properties will be
nil as a result, but with this patch, they may not be.  This still
seems strictly more correct, but do you know why this is okay?  Are
the properties nil at this point regardless because the button is all
in an overlay and we don't apply the :notmuch-part text property until
after this call to `notmuch-show-toggle-part-invisibility'?

Quoth Mark Walters on Sep 07 at 12:28 am:
> The lazy part handler had a bug that it allowed the button to be
> toggled to be specified. During toggling it needs to save and restore
> the text-properties for the button but it actually saved the text
> properties at point rather than from the button.
> 
> In almost all cases this didn't matter as as point had the same text
> properties as the button. However, it is a bug and did cause incorrect
> behaviour in some cases: see id:87txhz14z6.fsf@qmul.ac.uk for details.
> ---
> This is exactly the same as the patch in the parent except it has a
> commit message.
> 
> Best wishes
> 
> Mark
> 
> 
>  emacs/notmuch-show.el |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 20844f0..0267574 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -503,7 +503,7 @@ message at DEPTH in the current thread."
>  	     (new-start (button-start button))
>  	     (button-label (button-get button :base-label))
>  	     (old-point (point))
> -	     (properties (text-properties-at (point)))
> +	     (properties (text-properties-at (button-start button)))
>  	     (inhibit-read-only t))
>  	;; Toggle the button itself.
>  	(button-put button :notmuch-part-hidden (not show))

Re: [PATCH] emacs: show: lazy part handling bugfix

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> The lazy part handler had a bug that it allowed the button to be
> toggled to be specified. During toggling it needs to save and restore
> the text-properties for the button but it actually saved the text
> properties at point rather than from the button.

pushed,

d