In nsWebBrowserPersist::EndDownload set mCompleted = true before issuing the state stop notification

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

unspecified
mozilla49
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 fixed, firefox49 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
Bug 1101100 caused a regression in the SeaMonkey HTML Composer component. This can be fixed by moving |mCompleted = true| to before issuing the state stop notification.
Assignee

Comment 1

3 years ago
Try Server Job:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a0b6368e2cbc774aea890250d23dad81392c9b2&group_state=expanded&exclusion_profile=false

Hopefully I didn't regress Bug 1101100. It doesn't seem to have any tests attached so I can't tell.
Attachment #8747797 - Flags: review?(jld)
Comment on attachment 8747797 [details] [diff] [review]
Move the mCompleted line before state stop

Review of attachment 8747797 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable.

As for testing, I tried it on my local tree, and it doesn't break the basic e10s Firefox “Save As…” functionality, at least.  (There are a few automated tests of nsWebBrowserPersist in embedding/test, but not remotely as many as there should be in my opinion, and the ones that are there don't always check results as much as they could.)
Attachment #8747797 - Flags: review?(jld) → review+
https://hg.mozilla.org/mozilla-central/rev/da1c47951c03
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee

Comment 5

3 years ago
Comment on attachment 8747797 [details] [diff] [review]
Move the mCompleted line before state stop

Approval Request Comment
[Feature/regressing bug #]: Bug 1101100 - [e10s] Make “Save As… Complete Document” work in e10s tabs

[User impact if declined]: Impacts only SeaMonkey and Chatzilla

[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a0b6368e2cbc774aea890250d23dad81392c9b2&group_state=expanded&exclusion_profile=false

[Risks and why]: Risk is very low to none. Only SeaMonkey Composer component and Chatzilla bother to check the value returned. Nothing else does.

[String/UUID change made/needed]: None.
Attachment #8747797 - Flags: approval-mozilla-beta?
Attachment #8747797 - Flags: approval-mozilla-aurora?
Comment on attachment 8747797 [details] [diff] [review]
Move the mCompleted line before state stop

e10s will not ship on by default for 47, let's uplift this for Aurora48 only.
Attachment #8747797 - Flags: approval-mozilla-beta?
Attachment #8747797 - Flags: approval-mozilla-beta-
Attachment #8747797 - Flags: approval-mozilla-aurora?
Attachment #8747797 - Flags: approval-mozilla-aurora+

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.