Save Image from the long-press context menu is broken

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

unspecified
Firefox 47
Unspecified
All
Points:
---

Firefox Tracking Flags

(firefox47 verified, fennec47+)

Details

Attachments

(2 attachments)

STR:

1) Browse to some site with an image
2) Long-press the image, choose the "Image" tab, and do "Save Image As"

ER:

We should get UI for saving the image somewhere.

AR:

E/GeckoConsole(25669): [JavaScript Error: "Error: saveImageURL couldn't compute private state of content window" {file: "chrome://global/content/contentAreaUtils.js" line: 159}]

I broke this in bug 1243643, where I only accounted for the cases where saveImageURL was being handed a CPOW as a Doc, or being handed the aIsContentWindowPrivate bool. I failed to account for the case where we're being handed a Doc that's being rendered in the same process, which is the way things used to be.

I suspect this breaks other Gecko apps that rely on ContentAreaUtils, like SeaMonkey and Thunderbird.

The solution is to properly support the deprecated API. The long-term solution is for Fennec to pass in the aIsContentWindowPrivate bool.
(Assignee)

Updated

3 years ago
Blocks: 1243643

Updated

3 years ago
tracking-fennec: --- → ?
(Assignee)

Comment 1

3 years ago
Created attachment 8716505 [details]
MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws

Review commit: https://reviewboard.mozilla.org/r/33847/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33847/
Attachment #8716505 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 2

3 years ago
Created attachment 8716506 [details]
MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws

Review commit: https://reviewboard.mozilla.org/r/33849/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33849/
Attachment #8716506 - Flags: review?(margaret.leibovic)

Comment 3

3 years ago
"Bug 1246466 - Browser: Context Menu 'Save image as ...' does nothing" shows the same error message "chrome://global/content/contentAreaUtils.js"  -- Not only FF/Android affected? OS=All for now.
OS: Unspecified → All
See Also: → bug 1246466
Comment on attachment 8716505 [details]
MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws

https://reviewboard.mozilla.org/r/33847/#review30689

I don't feel confident reviewing contentAreaUtils.js changes, but I'll happily give this a rubber stamp if that's what you're looking for. 

Fun fact: this is one of our most used menu items. I wonder why I haven't heard anyone else report this bustage.
Attachment #8716505 - Flags: review?(margaret.leibovic) → review+

Updated

3 years ago
Attachment #8716506 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8716506 [details]
MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws

https://reviewboard.mozilla.org/r/33849/#review30693
(Assignee)

Comment 6

3 years ago
(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 8716505 [details]
> MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through
> saveImageURL properly. r?margaret
> 
> https://reviewboard.mozilla.org/r/33847/#review30689
> 
> I don't feel confident reviewing contentAreaUtils.js changes, but I'll
> happily give this a rubber stamp if that's what you're looking for. 

I'll take it! :D

> 
> Fun fact: this is one of our most used menu items. I wonder why I haven't
> heard anyone else report this bustage.

I wouldn't have guessed that, but I guess that's what makes Telemetry fascinating!
(In reply to :Margaret Leibovic from comment #4)

> Fun fact: this is one of our most used menu items. I wonder why I haven't
> heard anyone else report this bustage.

Probably most users just assume that the image was saved somewhere default :/
(Assignee)

Comment 8

3 years ago
Comment on attachment 8716505 [details]
MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33847/diff/1-2/
Attachment #8716505 - Attachment description: MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r?margaret → MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret
(Assignee)

Comment 9

3 years ago
Comment on attachment 8716506 [details]
MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33849/diff/1-2/
Attachment #8716506 - Attachment description: MozReview Request: Bug 1246244 - Regression test. r?margaret → MozReview Request: Bug 1246244 - Regression test. r=margaret
(Assignee)

Updated

3 years ago
Attachment #8716505 - Flags: review?(jld)
(Assignee)

Updated

3 years ago
Attachment #8716506 - Flags: review?(jld)
(In reply to Richard Newman [:rnewman] from comment #7)
> (In reply to :Margaret Leibovic from comment #4)
> 
> > Fun fact: this is one of our most used menu items. I wonder why I haven't
> > heard anyone else report this bustage.
> 
> Probably most users just assume that the image was saved somewhere default :/

I kept thinking it was gonna be fixed when I updated to the next Nightly and was slow to file a bug.
(Assignee)

Comment 11

3 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> (In reply to Richard Newman [:rnewman] from comment #7)
> > (In reply to :Margaret Leibovic from comment #4)
> > 
> > > Fun fact: this is one of our most used menu items. I wonder why I haven't
> > > heard anyone else report this bustage.
> > 
> > Probably most users just assume that the image was saved somewhere default :/
> 
> I kept thinking it was gonna be fixed when I updated to the next Nightly and
> was slow to file a bug.

Cool if I redirect the review request to you? I think jld is busy.
Flags: needinfo?(jaws)
I've looked at the patches and nothing jumped out at me as bad, but I don't understand frontend-land very well so I wanted to spend some more time on it.  I'm fine with handing off the r? if someone else has more time right now.
Assignee: nobody → mconley
tracking-fennec: ? → 47+
(Assignee)

Updated

3 years ago
Attachment #8716505 - Flags: review?(jld) → review?(jaws)
(Assignee)

Updated

3 years ago
Attachment #8716506 - Flags: review?(jld) → review?(jaws)
Comment on attachment 8716505 [details]
MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws

https://reviewboard.mozilla.org/r/33847/#review31253

::: toolkit/content/contentAreaUtils.js
(Diff revision 2)
> -    Deprecated.warning("saveImageURL should not be passed document CPOWs. " +
> -                       "The caller should pass in the content type and " +
> -                       "disposition themselves",
> -                       "https://bugzilla.mozilla.org/show_bug.cgi?id=1243643");
> -    if (aIsContentWindowPrivate == undefined) {

Shouldn't we still keep the deprecated warning around for when saveImageURL is passed a CPOW, just outside of us computing the state of aIsContentWindowPrivate?
(Assignee)

Comment 15

3 years ago
https://reviewboard.mozilla.org/r/33847/#review31253

> Shouldn't we still keep the deprecated warning around for when saveImageURL is passed a CPOW, just outside of us computing the state of aIsContentWindowPrivate?

Yeah, that's a good call. I'll put that back, and just make the test temporarily flip the forbid CPOWs pref for now (since Cu.isCrossProcessWrapper will get called outside of the addon compartment).
(Assignee)

Comment 16

3 years ago
Comment on attachment 8716505 [details]
MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33847/diff/2-3/
Attachment #8716505 - Attachment description: MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret → MozReview Request: Bug 1246244 - Allow non-CPOW documents to pass through saveImageURL properly. r=margaret,jaws
(Assignee)

Comment 17

3 years ago
Comment on attachment 8716506 [details]
MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33849/diff/2-3/
Attachment #8716506 - Attachment description: MozReview Request: Bug 1246244 - Regression test. r=margaret → MozReview Request: Bug 1246244 - Regression test. r=margaret,jaws

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61eec2456b97
https://hg.mozilla.org/mozilla-central/rev/24cd0b6089b2
https://hg.mozilla.org/mozilla-central/rev/7ecd788a20f1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Long press on the image, go to the "Image" tab and choose "Save Image As" -> A snackbar is displayed at the bottom of the page and a 'Download complete' notification is present in the android notification bar.
Verified as fixed using:
Device: One A2001 (Android 5.1.1) 
Build: Firefox for Android 47 Beta 1
status-firefox47: fixed → verified
You need to log in before you can comment on or make changes to this bug.