Bugzilla

For reference, these comments provide previous explanations of this proposed change:
bug 1796711 comment 3
bug 1796711 comment 4
bug 1796711 comment 18
bug 1796711 comment 29
Other comment in bug 1796711 may be relevant but discussions of POP3 there are not relevant to the current bug.

Since my last diff attachment at bug 1796711, I attempted, once more, to fix the existing fetch on demand feature without success. It appears that this is a feature that has never completely worked, especially when the attachment is on an email which itself is an attachment. Also, since ESR 78, the full message is always fetched, with ```mail.server.default.mime_parts_on_demand``` true or false. (Since ESR 78, mime_parts_on_demand has defaulted to false). So rather than trying any more to fix the "parts on demand" feature, I continued on with my previous efforts to completely eliminated fetch of parts on demand.

This bug, as reported by Virgo Pärna in comment 0, brought up an interesting variation in that PDF tabs left open from the previous session, on startup, produce immediate URLs that request the mime part for the PDF attachment. With the release code, the URL requested by the tab would often fail to generate the part within the body shell of the message leaving only the placeholder text "This body part will be downloaded on demand."  This text causes the 27 byte issue when the attachment is saved or opened.

Note:  Bug 570914 comment 86 shows how the 27 bytes are produced.  ```echo Thisbodypartwillbedownloadedondemand | base64 -d > t.txt``` produces ```t.txt```, a 27 byte file. The spaces and ending period are left out since they are not valid base64 values and most attachments are marked as base64 encoded and TB does something similar when it decodes and saves the empty part.

Anyhow, the tabs containing PDFs still produce a URL with part specifiers but the code now just obtains the part from the previously cached full message rather than asking the server for the part or looking for the part in a part-only cache entry. If the full message is not found in cache at startup, the full message is first fetched so the PDF part can be extracted, using TB's mime code, from the full message cache and then displayed or saved.

One issue I noticed was that often cache entries were getting invalidated (doomed) for no good reason that I could tell. This would happen, for example, when a message being read from cache is moved off of by the user before it is completely read from cache. This would signal a ```nsImapMockChannel::Cancel``` which checks that if a write is in progress a doom of the entry occurs. But there was no good check that a write was or wasn't in progress. It assumed that if a read wasn't in progress that a write must be in progress. I added the boolean variable ```nsImapMockChannel::mWritingToCache``` to know for sure when a cache write is in progress to avoid unnecessary dooming and subsequent imap re-downloads.

A primary change made is the use of cache2 disk instead of cache2 memory and I did a lot of tests with the new disk cache usage. As explained in comments referenced above, disk cache provides more storage with default settings so larger and more messages can be cached. But I was curious if memory cache would still work with my changes. The main selection of disk vs. memory cache is one line in ```nsImapService::GetCacheStorage```.  It still worked but I also had to change the "disk or memory" boolean parameter in calls to ```net::CacheObserver::EntryIsTooBig``` where I check if the message will fit in the cache entry. So after some more successful testing with memory cache I decided to make the cache2 type selectable with a new boolean pref ```mail.imap.use_disk_cache2``` defaulting to true. So to select memory cache instead, set the pref false. A TB restart *must* occur for this to take effect.

To do:

-I haven't tested the case where you have a "hybrid" storage setup where the newer messages are stored offline but the older ones are not. I need to make sure I haven't broken this and that only the older messages are cached.

-If memory cache is used, the cache is gone after TB restart. But with disk cache it stays put on TB restart or after system reboot and will be used in the next TB session. Possibly a new pref (and probably a UI setting) is needed to enable auto-delete of disk cache on tb shutdown.
Of course, a user can manually clear cache at any point during tb runtime in the General settings. This clears both disk and memory cache.

-I added a new "about:cache" items to the troubleshooting info screen. If this is kept, it also needs a label added, maybe "Cache Use", which I think requires a change to mozilla code.

-There is still code in place that reads in the "mime_parts_on_demand" prefs. These can be removed and their associated code removed if this change is accepted:
```mail.imap.mime_parts_on_demand```
```mail.imap.mime_parts_on_demand_threshold```
```mail.server.default.mime_parts_on_demand```

-If this change is accepted, these files can be completely removed:
```mailnews/imap/src/nsImapBodyShell.cpp```
```mailnews/imap/src/nsImapBodyShell.h```

For reference, these comments provide previous explanations of this proposed change:
bug 1796711 comment 3
bug 1796711 comment 4
bug 1796711 comment 18
bug 1796711 comment 29
Other comment in bug 1796711 may be relevant but discussions of POP3 there are not relevant to the current bug.

Since my last diff attachment at bug 1796711, I attempted, once more, to fix the existing fetch on demand feature without success. It appears that this is a feature that has never completely worked, especially when the attachment is on an email which itself is an attachment. Also, since ESR 78, the full message is always fetched, with ```mail.server.default.mime_parts_on_demand``` true or false. (Since ESR 78, mime_parts_on_demand has defaulted to false). So rather than trying any more to fix the "parts on demand" feature, I continued on with my previous efforts to completely eliminated fetch of parts on demand.

This bug, as reported by Virgo Pärna in comment 0, brought up an interesting variation in that PDF tabs left open from the previous session, on startup, produce immediate URLs that request the mime part for the PDF attachment. With the release code, the URL requested by the tab would often fail to generate the part within the body shell of the message leaving only the placeholder text "This body part will be downloaded on demand."  This text causes the 27 byte issue when the attachment is saved or opened.

Note:  Bug 570914 comment 86 shows how the 27 bytes are produced.  ```echo Thisbodypartwillbedownloadedondemand | base64 -d > t.txt``` produces ```t.txt```, a 27 byte file. The spaces and ending period are left out since they are not valid base64 values and most attachments are marked as base64 encoded and TB does something similar when it decodes and saves the empty part.

Anyhow, the tabs containing PDFs still produce a URL with part specifiers but the code now just obtains the part from the previously cached full message rather than asking the server for the part or looking for the part in a part-only cache entry. If the full message is not found in cache at startup, the full message is first fetched so the PDF part can be extracted, using TB's mime code, from the full message cache and then displayed or saved.

One issue I noticed was that often cache entries were getting invalidated (doomed) for no good reason that I could tell. This would happen, for example, when a message being read from cache is moved off of by the user before it is completely read from cache. This would signal a ```nsImapMockChannel::Cancel``` which checks that if a write is in progress a doom of the entry occurs. But there was no good check that a write was or wasn't in progress. It assumed that if a read wasn't in progress that a write must be in progress. I added the boolean variable ```nsImapMockChannel::mWritingToCache``` to know for sure when a cache write is in progress to avoid unnecessary dooming and subsequent imap re-downloads.

A primary change made is the use of cache2 disk instead of cache2 memory and I did a lot of tests with the new disk cache usage. As explained in comments referenced above, disk cache provides more storage with default settings so larger and more messages can be cached. But I was curious if memory cache would still work with my changes. The main selection of disk vs. memory cache is one line in ```nsImapService::GetCacheStorage```.  It still worked but I also had to change the "disk or memory" boolean parameter in calls to ```net::CacheObserver::EntryIsTooBig``` where I check if the message will fit in the cache entry. So after some more successful testing with memory cache I decided to make the cache2 type selectable with a new boolean pref ```mail.imap.use_disk_cache2``` defaulting to true. So to select memory cache instead, set the pref false. A TB restart *must* occur for this to take effect.

To do:

- I haven't tested the case where you have a "hybrid" storage setup where the newer messages are stored offline but the older ones are not. I need to make sure I haven't broken this and that only the older messages are cached.
**Tested this, looks OK**

- If memory cache is used, the cache is gone after TB restart. But with disk cache it stays put on TB restart or after system reboot and will be used in the next TB session. Possibly a new pref (and probably a UI setting) is needed to enable auto-delete of disk cache on tb shutdown.
Of course, a user can manually clear cache at any point during tb runtime in the General settings. This clears both disk and memory cache.

- I added a new "about:cache" items to the troubleshooting info screen. If this is kept, it also needs a label added, maybe "Cache Use", which I think requires a change to mozilla code.

- There is still code in place that reads in the "mime_parts_on_demand" prefs. These can be removed and their associated code removed if this change is accepted:
```mail.imap.mime_parts_on_demand```
```mail.imap.mime_parts_on_demand_threshold```
```mail.server.default.mime_parts_on_demand```
**Removed these prefs and associated code in several files. Also removed code to set/getContentModified in URL and in cache metadata since this is only relevant when message parts are fetched and cached.**

- If this change is accepted, these files can be completely removed:
```mailnews/imap/src/nsImapBodyShell.cpp```
```mailnews/imap/src/nsImapBodyShell.h```

- Evaluate failing unit tests and remove them if no longer applicable.

For reference, these comments provide previous explanations of this proposed change:
bug 1796711 comment 3
bug 1796711 comment 4
bug 1796711 comment 18
bug 1796711 comment 29
Other comment in bug 1796711 may be relevant but discussions of POP3 there are not relevant to the current bug.

Since my last diff attachment at bug 1796711, I attempted, once more, to fix the existing fetch on demand feature without success. It appears that this is a feature that has never completely worked, especially when the attachment is on an email which itself is an attachment. Also, since ESR 78, the full message is always fetched, with ```mail.server.default.mime_parts_on_demand``` true or false. (Since ESR 78, mime_parts_on_demand has defaulted to false). So rather than trying any more to fix the "parts on demand" feature, I continued on with my previous efforts to completely eliminated fetch of parts on demand.

This bug, as reported by Virgo Pärna in comment 0, brought up an interesting variation in that PDF tabs left open from the previous session, on startup, produce immediate URLs that request the mime part for the PDF attachment. With the release code, the URL requested by the tab would often fail to generate the part within the body shell of the message leaving only the placeholder text "This body part will be downloaded on demand."  This text causes the 27 byte issue when the attachment is saved or opened.

Note:  Bug 570914 comment 86 shows how the 27 bytes are produced.  ```echo Thisbodypartwillbedownloadedondemand | base64 -d > t.txt``` produces ```t.txt```, a 27 byte file. The spaces and ending period are left out since they are not valid base64 values and most attachments are marked as base64 encoded and TB does something similar when it decodes and saves the empty part.

Anyhow, the tabs containing PDFs still produce a URL with part specifiers but the code now just obtains the part from the previously cached full message rather than asking the server for the part or looking for the part in a part-only cache entry. If the full message is not found in cache at startup, the full message is first fetched so the PDF part can be extracted, using TB's mime code, from the full message cache and then displayed or saved.

One issue I noticed was that often cache entries were getting invalidated (doomed) for no good reason that I could tell. This would happen, for example, when a message being read from cache is moved off of by the user before it is completely read from cache. This would signal a ```nsImapMockChannel::Cancel``` which checks that if a write is in progress a doom of the entry occurs. But there was no good check that a write was or wasn't in progress. It assumed that if a read wasn't in progress that a write must be in progress. I added the boolean variable ```nsImapMockChannel::mWritingToCache``` to know for sure when a cache write is in progress to avoid unnecessary dooming and subsequent imap re-downloads.

A primary change made is the use of cache2 disk instead of cache2 memory and I did a lot of tests with the new disk cache usage. As explained in comments referenced above, disk cache provides more storage with default settings so larger and more messages can be cached. But I was curious if memory cache would still work with my changes. The main selection of disk vs. memory cache is one line in ```nsImapService::GetCacheStorage```.  It still worked but I also had to change the "disk or memory" boolean parameter in calls to ```net::CacheObserver::EntryIsTooBig``` where I check if the message will fit in the cache entry. So after some more successful testing with memory cache I decided to make the cache2 type selectable with a new boolean pref ```mail.imap.use_disk_cache2``` defaulting to true. So to select memory cache instead, set the pref false. A TB restart *must* occur for this to take effect.

To do:

- I haven't tested the case where you have a "hybrid" storage setup where the newer messages are stored offline but the older ones are not. I need to make sure I haven't broken this and that only the older messages are cached.
**Tested this, looks OK**

- If memory cache is used, the cache is gone after TB restart. But with disk cache it stays put on TB restart or after system reboot and will be used in the next TB session. Possibly a new pref (and probably a UI setting) is needed to enable auto-delete of disk cache on tb shutdown.
Of course, a user can manually clear cache at any point during tb runtime in the General settings. This clears both disk and memory cache.
**Added a "Clear cache on shutdown" checkbox to General settings | Disk Space defaulted to not checked. When checked it sets the existing mozilla pref (was unused in tb) ```privacy.clearOnShutdown.cache``` which is now looked at on tb shutdown and if true, the cache is cleared in the same way as the "Clear Now" button clears cache (calls cache2 service function Clear).**

- I added a new "about:cache" items to the troubleshooting info screen. If this is kept, it also needs a label added, maybe "Cache Use", which I think requires a change to mozilla code.

- There is still code in place that reads in the "mime_parts_on_demand" prefs. These can be removed and their associated code removed if this change is accepted:
```mail.imap.mime_parts_on_demand```
```mail.imap.mime_parts_on_demand_threshold```
```mail.server.default.mime_parts_on_demand```
**Removed these prefs and associated code in several files. Also removed code to set/getContentModified in URL and in cache metadata since this is only relevant when message parts are fetched and cached.**

- If this change is accepted, these files can be completely removed:
```mailnews/imap/src/nsImapBodyShell.cpp```
```mailnews/imap/src/nsImapBodyShell.h```

- Evaluate failing unit tests and remove them if no longer applicable.

For reference, these comments provide previous explanations of this proposed change:
bug 1796711 comment 3
bug 1796711 comment 4
bug 1796711 comment 18
bug 1796711 comment 29
Other comment in bug 1796711 may be relevant but discussions of POP3 there are not relevant to the current bug.

Since my last diff attachment at bug 1796711, I attempted, once more, to fix the existing fetch on demand feature without success. It appears that this is a feature that has never completely worked, especially when the attachment is on an email which itself is an attachment. Also, since ESR 78, the full message is always fetched, with ```mail.server.default.mime_parts_on_demand``` true or false. (Since ESR 78, mime_parts_on_demand has defaulted to false). So rather than trying any more to fix the "parts on demand" feature, I continued on with my previous efforts to completely eliminated fetch of parts on demand.

This bug, as reported by Virgo Pärna in comment 0, brought up an interesting variation in that PDF tabs left open from the previous session, on startup, produce immediate URLs that request the mime part for the PDF attachment. With the release code, the URL requested by the tab would often fail to generate the part within the body shell of the message leaving only the placeholder text "This body part will be downloaded on demand."  This text causes the 27 byte issue when the attachment is saved or opened.

Note:  Bug 570914 comment 86 shows how the 27 bytes are produced.  ```echo Thisbodypartwillbedownloadedondemand | base64 -d > t.txt``` produces ```t.txt```, a 27 byte file. The spaces and ending period are left out since they are not valid base64 values and most attachments are marked as base64 encoded and TB does something similar when it decodes and saves the empty part.

Anyhow, the tabs containing PDFs still produce a URL with part specifiers but the code now just obtains the part from the previously cached full message rather than asking the server for the part or looking for the part in a part-only cache entry. If the full message is not found in cache at startup, the full message is first fetched so the PDF part can be extracted, using TB's mime code, from the full message cache and then displayed or saved.

One issue I noticed was that often cache entries were getting invalidated (doomed) for no good reason that I could tell. This would happen, for example, when a message being read from cache is moved off of by the user before it is completely read from cache. This would signal a ```nsImapMockChannel::Cancel``` which checks that if a write is in progress a doom of the entry occurs. But there was no good check that a write was or wasn't in progress. It assumed that if a read wasn't in progress that a write must be in progress. I added the boolean variable ```nsImapMockChannel::mWritingToCache``` to know for sure when a cache write is in progress to avoid unnecessary dooming and subsequent imap re-downloads.

A primary change made is the use of cache2 disk instead of cache2 memory and I did a lot of tests with the new disk cache usage. As explained in comments referenced above, disk cache provides more storage with default settings so larger and more messages can be cached. But I was curious if memory cache would still work with my changes. The main selection of disk vs. memory cache is one line in ```nsImapService::GetCacheStorage```.  It still worked but I also had to change the "disk or memory" boolean parameter in calls to ```net::CacheObserver::EntryIsTooBig``` where I check if the message will fit in the cache entry. So after some more successful testing with memory cache I decided to make the cache2 type selectable with a new boolean pref ```mail.imap.use_disk_cache2``` defaulting to true. So to select memory cache instead, set the pref false. A TB restart *must* occur for this to take effect.

To do:

- I haven't tested the case where you have a "hybrid" storage setup where the newer messages are stored offline but the older ones are not. I need to make sure I haven't broken this and that only the older messages are cached.
**Tested this, looks OK**

- If memory cache is used, the cache is gone after TB restart. But with disk cache it stays put on TB restart or after system reboot and will be used in the next TB session. Possibly a new pref (and probably a UI setting) is needed to enable auto-delete of disk cache on tb shutdown.
Of course, a user can manually clear cache at any point during tb runtime in the General settings. This clears both disk and memory cache.
**Added a "Clear cache on shutdown" checkbox to General settings | Disk Space defaulted to not checked. When checked it sets the existing mozilla pref (was unused in tb) ```privacy.clearOnShutdown.cache``` which is now looked at on tb shutdown and if true, the cache is cleared in the same way as the "Clear Now" button clears cache (calls cache2 service function Clear).**

- I added a new "about:cache" items to the troubleshooting info screen. If this is kept, it also needs a label added, maybe "Cache Use", which I think requires a change to mozilla code.

- There is still code in place that reads in the "mime_parts_on_demand" prefs. These can be removed and their associated code removed if this change is accepted:
```mail.imap.mime_parts_on_demand```
```mail.imap.mime_parts_on_demand_threshold```
```mail.server.default.mime_parts_on_demand```
**Removed these prefs and associated code in several files. Also removed code to set/getContentModified in URL and in cache metadata since this is only relevant when message parts are fetched and cached.**

- If this change is accepted, these files can be completely removed:
```mailnews/imap/src/nsImapBodyShell.cpp```
```mailnews/imap/src/nsImapBodyShell.h```
**For now, I've just remove nsImapBodyShell.cpp from the moz.build**

- Evaluate failing unit tests and remove them if no longer applicable.
**Set the three unit test pertaining to parts on demand, caching of parts and download  on demand to be skipped.**

For reference, these comments provide previous explanations of this proposed change:
bug 1796711 comment 3
bug 1796711 comment 4
bug 1796711 comment 18
bug 1796711 comment 29
Other comment in bug 1796711 may be relevant but discussions of POP3 there are not relevant to the current bug.

Since my last diff attachment at bug 1796711, I attempted, once more, to fix the existing fetch on demand feature without success. It appears that this is a feature that has never completely worked, especially when the attachment is on an email which itself is an attachment. Also, since ESR 78, the full message is always fetched, with ```mail.server.default.mime_parts_on_demand``` true or false. (Since ESR 78, mime_parts_on_demand has defaulted to false). So rather than trying any more to fix the "parts on demand" feature, I continued on with my previous efforts to completely eliminated fetch of parts on demand.

This bug, as reported by Virgo Pärna in comment 0, brought up an interesting variation in that PDF tabs left open from the previous session, on startup, produce immediate URLs that request the mime part for the PDF attachment. With the release code, the URL requested by the tab would often fail to generate the part within the body shell of the message leaving only the placeholder text "This body part will be downloaded on demand."  This text causes the 27 byte issue when the attachment is saved or opened.

Note:  Bug 570914 comment 86 shows how the 27 bytes are produced.  ```echo Thisbodypartwillbedownloadedondemand | base64 -d > t.txt``` produces ```t.txt```, a 27 byte file. The spaces and ending period are left out since they are not valid base64 values and most attachments are marked as base64 encoded and TB does something similar when it decodes and saves the empty part.

Anyhow, the tabs containing PDFs still produce a URL with part specifiers but the code now just obtains the part from the previously cached full message rather than asking the server for the part or looking for the part in a part-only cache entry. If the full message is not found in cache at startup, the full message is first fetched so the PDF part can be extracted, using TB's mime code, from the full message cache and then displayed or saved.

One issue I noticed was that often cache entries were getting invalidated (doomed) for no good reason that I could tell. This would happen, for example, when a message being read from cache is moved off of by the user before it is completely read from cache. This would signal a ```nsImapMockChannel::Cancel``` which checks that if a write is in progress a doom of the entry occurs. But there was no good check that a write was or wasn't in progress. It assumed that if a read wasn't in progress that a write must be in progress. I added the boolean variable ```nsImapMockChannel::mWritingToCache``` to know for sure when a cache write is in progress to avoid unnecessary dooming and subsequent imap re-downloads.

A primary change made is the use of cache2 disk instead of cache2 memory and I did a lot of tests with the new disk cache usage. As explained in comments referenced above, disk cache provides more storage with default settings so larger and more messages can be cached. But I was curious if memory cache would still work with my changes. The main selection of disk vs. memory cache is one line in ```nsImapService::GetCacheStorage```.  It still worked but I also had to change the "disk or memory" boolean parameter in calls to ```net::CacheObserver::EntryIsTooBig``` where I check if the message will fit in the cache entry. So after some more successful testing with memory cache I decided to make the cache2 type selectable with a new boolean pref ```mail.imap.use_disk_cache2``` defaulting to true. So to select memory cache instead, set the pref false. A TB restart *must* occur for this to take effect.

To do:

- I haven't tested the case where you have a "hybrid" storage setup where the newer messages are stored offline but the older ones are not. I need to make sure I haven't broken this and that only the older messages are cached.
**Tested this, looks OK**

- If memory cache is used, the cache is gone after TB restart. But with disk cache it stays put on TB restart or after system reboot and will be used in the next TB session. Possibly a new pref (and probably a UI setting) is needed to enable auto-delete of disk cache on tb shutdown.
Of course, a user can manually clear cache at any point during tb runtime in the General settings. This clears both disk and memory cache.
**Added a "Clear cache on shutdown" checkbox to General settings | Disk Space defaulted to not checked. When checked it sets the existing mozilla pref (was unused in tb) ```privacy.clearOnShutdown.cache``` which is now looked at on tb shutdown and if true, the cache is cleared in the same way as the "Clear Now" button clears cache (calls cache2 service function Clear).**

- I added a new "about:cache" items to the troubleshooting info screen. If this is kept, it also needs a label added, maybe "Cache Use", which I think requires a change to mozilla code.

- There is still code in place that reads in the "mime_parts_on_demand" prefs. These can be removed and their associated code removed if this change is accepted:
```mail.imap.mime_parts_on_demand```
```mail.imap.mime_parts_on_demand_threshold```
```mail.server.default.mime_parts_on_demand```
**Removed these prefs and associated code in several files. Also removed code to set/getContentModified in URL and in cache metadata since this is only relevant when message parts are fetched and cached.**

- If this change is accepted, these files can be completely removed:
```mailnews/imap/src/nsImapBodyShell.cpp```
```mailnews/imap/src/nsImapBodyShell.h```
**For now, I've just remove nsImapBodyShell.cpp from the moz.build**

- Evaluate failing unit tests and remove them if no longer applicable.
**Set the three unit test pertaining to parts on demand, caching of parts and download  on demand to be skipped.**
**Edit: Magnus said to not skip them but just remove the reference to the test files from the \*.ini test driving files. But like the nsImapBodyShell.\* files, the test files are still present in the project source tree.**