201610 - nsWebBrowserPersist::CancelSave doesn't cancel uploads (publishing)

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[jag (Peter Annema)]

It looks like the EnumCleanupUploadList callback (and prolly some other stuff)
needs some work to fetch the channel for the upload and cancel it.

Before testing on this can be done, bug 201609 needs to be fixed. To test,
create a simple html file with a biiiiig picture embedded, log into your
destination ftp site, and publish the html + img there. Cancel publishing, and
notice (using e.g. ls) how the filesize keeps growing way past you pressing
cancel, up to the full filesize.

Comment 1

[kinmoz]

--> adamlock

Comment 2

[sairuh (rarely reading bugmail)]

publishing -> chris.

Comment 3

[Samir Gehani]

Editor triage team: nsbeta1+/adt2

Comment 4

[Adam Lock]

The UploadList uses the channel as the hash key, so the EnumCleanupUploadList
function should probably do something similar to EnumCleanupOutputMap, calling
channel->Cancel(NS_BINDING_ABORTED) but using the channel QI'd from the
nsISupportsKey associated with each item.

Comment 5

[jag (Peter Annema)]

Sounds good. Will you be able to get around to this before 1.4b?

Comment 6

[Adam Lock]

Attachment: Patch

Patch does what I suggested previously, canceling the channel stored in the
hash key

Comment 7

[Adam Lock]

Comment on attachment 120829 [details] [diff] [review]
Patch

Requesting r/sr on this

Comment 8

[Kathleen :Brade]

Comment on attachment 120829 [details] [diff] [review]
Patch

r=brade for these changes

I think we probably also want to check mCancel in this block:
676	if (mOutputMap.Count() == 0 && mUploadList.Count() == 0)
677	{
678	    // if no documents left in mDocList, --> done
679	    // if we have no files left to serialize and no error result, -->
done
680	    if (mDocList.Count() == 0
681		|| (!SerializeNextFile() && NS_SUCCEEDED(mPersistResult)))
682	    {
683		completed = PR_TRUE;
684	    }
685	}

That can be done as a separate patch if preferred.

Comment 9

[jag (Peter Annema)]

Comment on attachment 120829 [details] [diff] [review]
Patch

sr=jag

Comment 10

[Adam Lock]

Patch was checked in today. I'll leave the bug open to cover the other issue.

Comment 11

[Paul Wyskoczka]

ADT: Nominating topembed

Comment 12

[Paul Wyskoczka]

ADT: Nominating topembed

Comment 13

[Adam Lock]

Comment on attachment 120829 [details] [diff] [review]
Patch

Requesting 1.4 checkin approval. Fixes cancel in composer upload operations

Comment 14

[jag (Peter Annema)]

That patch was already checked in. This bug was left open to deal with the other
issue brade brought up in comment 8.

Comment 15

[Adam Lock]

Comment on attachment 120829 [details] [diff] [review]
Patch

Doh! I should read my bugs more carefully in future

Comment 16

[Michael Buckland]

Discussed in topembed bug triage.  Plussing.  This plus is for comment 8.

Comment 17

[Adam Lock]

Attachment: Patch 2

This patch protects the SerializeNextFile() call by checking mCancel.

Comment 18

[Adam Lock]

Comment on attachment 123888 [details] [diff] [review]
Patch 2

Requesting r/sr on this simple check for !mCancel around the call to
SerializeNextFile

Comment 19

[jag (Peter Annema)]

Comment on attachment 123888 [details] [diff] [review]
Patch 2

sr=jag

Comment 20

[Kathleen :Brade]

Comment on attachment 123888 [details] [diff] [review]
Patch 2

r=brade
seeking a= for 1.4final to fix cancellation issues; this is a simple 1-line fix

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 123888 [details] [diff] [review]
Patch 2

a=asa (on behalf of drivers) for checkin to 1.4.

Comment 22

[Adam Lock]

Fix is checked in

Comment 23

[Chris Petersen]

Verified on trunk (2003-06-11-08) and branch (2003-06-11-05) Macho and win32
branch builds.