1246244 - Save Image from the long-press context menu is broken

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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/

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: 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/

Comment 3

[Rainer Bielefeld]

"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.

Comment 4

[:Margaret Leibovic]

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.

Comment 5

[:Margaret Leibovic]

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

https://reviewboard.mozilla.org/r/33849/#review30693

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(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!

Comment 7

[Richard Newman [:rnewman]]

(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 :/

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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/

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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/

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 12

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

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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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

https://reviewboard.mozilla.org/r/33849/#review31255

Comment 15

[Mike Conley (:mconley) (:⚙️)]

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).

Comment 16

[Mike Conley (:mconley) (:⚙️)]

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/

Comment 17

[Mike Conley (:mconley) (:⚙️)]

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/

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61eec2456b97
https://hg.mozilla.org/integration/mozilla-inbound/rev/24cd0b6089b2

Comment 19

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c13ff8c5a97a

Comment 20

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ecd788a20f1de92f075b3984406c7f556e1b473
Bug 1246244 - Follow-up - clean up MockFilePicker once test is done. r=me

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61eec2456b97
https://hg.mozilla.org/mozilla-central/rev/24cd0b6089b2
https://hg.mozilla.org/mozilla-central/rev/7ecd788a20f1

Comment 22

[Teodora Vermesan (:TeoVermesan)]

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