383716 - nsWebBrowserPersist::OnStopRequest doesn't check |status| for failure

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Ed Lee :Mardak]

Attachment: v1

It's possible to get a status failure OnStop, but |status| is only used when passing to |mProgressListener|.

Perhaps do something similar to exthandler's OnStop?

http://mxr.mozilla.org/mozilla/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2129

If there is a failure, a the |mProgressListener| will get a StatusChange with the status and error string; otherwise, it'll get a StateChange (stop) like usual.

Question is.. how many failures are actually arriving OnStop that didn't appear OnDataAvail.

Comment 1

[Ed Lee :Mardak]

This is necessary for the download manager to get the Link Fingerprint failure status as a StateChange.

Comment 2

[Ed Lee :Mardak]

Attachment: v2

- Fix nits (comments, void)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Edward, you'll request reviews once you're happy with the patch, right?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 269539 [details] [diff] [review]
v2

- I think it makes more sense to call EndDownload instead of Cancel
- don't cast to void
- you need to continue in this function, because Cancel does not set mEventSink etc to null

Comment 5

[Ed Lee :Mardak]

Attachment: v3

(In reply to comment #4)
> - you need to continue in this function, because Cancel does not set mEventSink
> etc to null
Done. Instead of returning, it'll reuse the existing code to |delete data| which will get |completed| set to true allowing us to...

> - I think it makes more sense to call EndDownload instead of Cancel
... reuse the EndDownload + nulling. Done.

> - don't cast to void
Removed.

Comment 6

[Ed Lee :Mardak]

Attachment: v4

Check if we haven't gotten a failure Result instead of checking if we haven't been canceled yet.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273208 [details] [diff] [review]
v4

Seems reasonable.

Comment 8

[Nickolay_Ponomarev]

Checking in embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp;
/cvsroot/mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp,v  <--  nsWebBrowserPersist.cpp
new revision: 1.130; previous revision: 1.129
done