1224437 - No "Publishing completed" status after green tick, publishing does not terminate

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[Lee P]

Attachment: Sea Monkey bug.docx

Publishing issue: Note evident until latest update.

File -> page edit -> publish -> green tick OK -> BUT CONTINUES TO PUBLISH. Only way to get out of the process is to cancel -> cancel -> cancel. When all cancelled can reload and all changes have been actioned.
Also the edit page is now an unknown entity - don't know if in edit page or not.
Please give some advice.
Thanks
Lee

Comment 1

[:Gijs (he/him)]

Not a security bug.

Comment 2

[Rainer Bielefeld]

NOT reproducible REPRODUCIBLE with  SeaMonkey German 2.39 final Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0 from official download area)  Gecko/20100101  Firefox/42.0  Build 20151103191810  (Classic Theme) on German WIN7 64bit with:

0. Download Attachment 8673077 [details] for Bug 1214005
1. Launch Composer → open file → add 1 character to first text line → click
   Publish-Icon'
   » Publish dialog appears
2. If necessary add user data for ftp upload → check "include images" in Publish
   TAB → [Publish]
   » Publishing to my ftp web space
   » after short moment green checkmark and status info "Publishing completed"
     appears

@Reporter: Please contribute additional information due to <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Bug_writing_guidelines>  (containing every key press and every mouse click) how to reproduce the problem reliably:
a) Test document
b) free ftp upload account or similar
c) Results with safe modes (disabled add-ons) or newly created user profile?
d) until what version did that work for you?
Please answer without citing, simply start your reply like
a): <answer>

Comment 3

[Rabbe Fogelholm]

The faulty behaviour is also present in the Seamonkey packaged for Ubuntu: 2.39-0ubuntu1 (stable), http://ubuntuzilla.sourceforge.net/.

The problem seems to have come with version 2.39. My temporary solution is to use a 2.38 version instead, e g https://archive.mozilla.org/pub/mozilla.org/seamonkey/releases/2.38/contrib/seamonkey-2.38.en-US.linux-x86_64.tar.bz2

Comment 4

[Lee P]

From Lee P again.
Since my original posting and reverting back to 2.38 all is OK on my PC.
However we are now attempting to edit a website with SeaMonkey Composer 2.39 using an Apple Mac.
We expected that this faulty behaviour would not be evident on the Apple Mac.
However the same problem "[Bug 1224437] No "Publishing completed" status after green tick, publishing does not terminate" is eveident on the Apple Mac.

When will 2.39' be bug free of this problem?
Thanks
Lee

Comment 5

[Rainer Bielefeld]

OS = All due to comments.
Useful instruction still missing.
Any chance to get a test account for one of the ftp upload areas where the problem appears?
We need information due to comment 2.

Comment 6

[Frank-Rainer Grahl (:frg)]

I can confirm this too with a private Windows build.

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0 SeaMonkey/2.41 en-uUS

I traced it back to 

editor\ui\composer\content\ComposerCommands.js

>> // STATE_IS_NETWORK signals end of publishing, as does the gPersistObj.currentState
>> if (aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK
>> && gPersistObj.currentState == nsIWebBrowserPersist.PERSIST_STATE_FINISHED)

Starting with 2.39 I never observed STATE_IS_NETWORK and PERSIST_STATE_FINISHED together so the if never evaluates to true. I am not sure if this isn't due to e10s changes in the backend. The comment indicated that both conditions signaled the end of publishing so as a test I used || instead of && which worked fine but I am more or less clueless about the apis involved so just consider it trial and error and not a fix.

FRG

Comment 7

[Chuck Baker]

I am able to duplicate this issue.  If necessary, I can provide a test ftp account to test with.

Comment 8

[Lee P]

From Lee P
Now that the bug has been duplicated by Chuck do you guys still need the info requested by Rainer?
Since I uninstalled 2.39 reinstalled 2.38 it has been impossible to re-create the 2.39 bug process. However if it's absolutely necessary I will upgrade to 2.39 and document the bug process for you.
Then I will uninstall 2.39 and reinstall 2.38 as the workaround.

Comment 9

[Rainer Bielefeld]

Frank-Rainer Grahl was able to reproduce the problem, so NEW, currently no more information required.

Comment 10

[Rainer Bielefeld]

I also observed the problem with SM 2.39 (?) few days ago, not remembering this Bug.

Comment 11

[Dan Dunay]

If you want this bug fixed, you need to vote it up at https://bugzilla.mozilla.org/show_bug.cgi?id=1224437 . As of now, there are only 3 votes (including mine) to get this fixed. No wonder it hasn't been fixed.

Comment 12

[Lee P]

Come on guys, please fix. It's a very old and unacceptable bug when operating Sea Monkey.
Thanks, hoping for a quick result.
Lee P

Comment 13

[Alan S.]

I just signed up so that I could add my vote to get this bug squashed. 

I've gone back to version 2.38 (Mac) and the publishing is going smoothly again.

Alan S

Comment 14

[Frank-Rainer Grahl (:frg)]

Attachment: 1224437-state.patch

I am quite sure this was broken by the changes in Bug 1101100.

I think the PERSIST_STATE_FINISHED state can no longer be used bere because it is set after the state cange to STATE_IS_NETWORK in EndDownload 8which is actually an upload here...). The result is already set and only some cleanup remains so the code should not fail.


>> nsWebBrowserPersist::EndDownload(nsresult aResult)
>> {
>>  // Store the error code in the result if it is an error
>>  if (NS_SUCCEEDED(mPersistResult) && NS_FAILED(aResult))
>>  {
>>      mPersistResult = aResult;
>>  }
>>
>>  // State stop notification
>>  if (mProgressListener) {
>>      mProgressListener->OnStateChange(nullptr, nullptr,
>>          nsIWebProgressListener::STATE_STOP
>>          | nsIWebProgressListener::STATE_IS_NETWORK, mPersistResult);
>>  }
>>
>>    // Do file cleanup if required
>>    if (NS_FAILED(aResult) && (mPersistFlags & 
>>         PERSIST_FLAGS_CLEANUP_ON_FAILURE))
>>    {
>>        CleanupLocalFiles();
>>    }
>>
>>    // Cleanup the channels
>>    mCompleted = true;
>>    Cleanup();

Teste on c-c VS2015 x64 private en-US build.

Published one html with one image to an ftp site and to a local file destination. Both worked.

>> User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101
>> Firefox/48.0 SeaMonkey/2.45a1
>> Build identifier: 20160414231315

As stated I do not know much about the apis involved so it might break something else when publishing.

Comment 15

[Philip Chee]

(In reply to Frank-Rainer Grahl from comment #14)
> Created attachment 8741538 [details] [diff] [review]
> 1224437-state.patch
> 
> I am quite sure this was broken by the changes in Bug 1101100.
Or Followup Bug 1207864 https://hg.mozilla.org/mozilla-central/rev/44c1e75e7266
(I think Chatzilla is also affected)

> I think the PERSIST_STATE_FINISHED state can no longer be used here because
> it is set after the state change to STATE_IS_NETWORK in EndDownload which is
> actually an upload here...). The result is already set and only some cleanup
> remains so the code should not fail.
> 
> 
> >> nsWebBrowserPersist::EndDownload(nsresult aResult)
> >> {
> >>  // Store the error code in the result if it is an error
> >>  if (NS_SUCCEEDED(mPersistResult) && NS_FAILED(aResult))
> >>  {
> >>      mPersistResult = aResult;
> >>  }
> >>
> >>  // State stop notification
> >>  if (mProgressListener) {
> >>      mProgressListener->OnStateChange(nullptr, nullptr,
> >>          nsIWebProgressListener::STATE_STOP
> >>          | nsIWebProgressListener::STATE_IS_NETWORK, mPersistResult);
> >>  }
> >>
> >>    // Do file cleanup if required
> >>    if (NS_FAILED(aResult) && (mPersistFlags & 
> >>         PERSIST_FLAGS_CLEANUP_ON_FAILURE))
> >>    {
> >>        CleanupLocalFiles();
> >>    }
> >>
> >>    // Cleanup the channels
> >>    mCompleted = true;
> >>    Cleanup();
> 
> Teste on c-c VS2015 x64 private en-US build.
> 
> Published one html with one image to an ftp site and to a local file
> destination. Both worked.
> 
> >> User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101
> >> Firefox/48.0 SeaMonkey/2.45a1
> >> Build identifier: 20160414231315
> 
> As stated I do not know much about the apis involved so it might break
> something else when publishing.

NI: Jed Davis
Is it correct that we should stop checking for nsIWebBrowserPersist.PERSIST_STATE_FINISHED within onStateChange()?

Comment 16

[Philip Chee]

Please check:
https://bugzilla.mozilla.org/showdependencytree.cgi?id=1101100&maxdepth=1&hide_resolved=0
For any other changes we need to port to SeaMonkey. Thanks.

Comment 17

[Philip Chee]

(In reply to Philip Chee from comment #15)

> NI: Jed Davis
> Is it correct that we should stop checking for
> nsIWebBrowserPersist.PERSIST_STATE_FINISHED within onStateChange()?

No reply from Jed Davis
NI => [e-mail]

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(Sorry; still dealing with my backlog from being away last week.)

My intent in bug 1101100 was to change the behavior of nsWebBrowserPersist as little as possible while making it multiprocess-capable, but there's basically nothing in the way of unit tests to help ensure that, and the documentation on the various state/status flags is somewhat sparse and unclear.  So if anything changed as far as what flags are set, it's probably a bug in Gecko.  But it sounds like it might be easier to just change the frontend?

Comment 19

[Philip Chee]

FRG: what if you backout http://hg.mozilla.org/mozilla-central/rev/44c1e75e7266 (Bug 1207864)

Comment 20

[Frank-Rainer Grahl (:frg)]

I can try but I am quite sure this wouldn't be a good idea because the result for mProgessListener would be undefined then. 

What should work is to put 

mCompleted = true;

before the state stop notification.

Comment 21

[Philip Chee]

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a0b6368e2cbc774aea890250d23dad81392c9b2

Comment 22

[Frank-Rainer Grahl (:frg)]

The treeherder build looks like it did ok. 

I tried it locally and it fixed it too. Reverted the patch and moved mCompleted before the start stop notification. A quick publish with an html and image to an ftp site worked ok.

Even if this is a gecko bug I would still drop checking for PERSIST_STATE_FINISHED as it seems that the flags are now really redundant here.

Comment 23

[Philip Chee]

(In reply to Frank-Rainer Grahl from comment #22)
> The treeherder build looks like it did ok. 
> 
> I tried it locally and it fixed it too. Reverted the patch and moved
> mCompleted before the start stop notification. A quick publish with an html
> and image to an ftp site worked ok.
> 
> Even if this is a gecko bug I would still drop checking for
> PERSIST_STATE_FINISHED as it seems that the flags are now really redundant
> here.
PERSIST_STATE_FINISHED is used in several other places in ComposerCommands.js so I'm going to be conservative here. When Neil comes back we can ask him to decide.

Comment 24

[Philip Chee]

Comment on attachment 8741538 [details] [diff] [review]
1224437-state.patch

r=me for comm- branches wherever Mozilla Bug 1269412 doesn't land.

Comment 25

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-beta/rev/304b2fd17d04
https://hg.mozilla.org/releases/comm-release/rev/4494632520a4

Didn't push to comm-esr45.

Comment 26

[Frank-Rainer Grahl (:frg)]

Pushed to comm-esr45 as per todays status meeting:

https://hg.mozilla.org/releases/comm-esr45/rev/4ad1905323d8