1135933 - [e10s] "Set As Desktop Background..." in remote browser causes unsafe CPOW usage warning, TypeErrors, and then a NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE if you try to set it

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Ian Moody [:Kwan] (UTC+1)]

+++ This bug was initially created as a clone of Bug #1133577 +++

STR:

1) Visit a site with some images on it in an e10s window
2) Right-click on an image, and choose "Set As Desktop Background..."

This causes some "unsafe CPOW usage" warnings in the Browser Console, some TypeErrors, and if you then hit "Set Desktop Background" in the dialog window, you get a "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE".

In browser/base/content/nsContextMenu.js:

  disableSetDesktopBackground: function() {
    // Disable the Set as Desktop Background menu item if we're still trying
    // to load the image or the load failed.
    if (!(this.target instanceof Ci.nsIImageLoadingContent)) <- Causes CPOW warning
      return true;

    if (("complete" in this.target) && !this.target.complete) <- Causes CPOW warning
      return true;

    if (this.target.currentURI.schemeIs("javascript")) <- Causes CPOW warning
      return true;

    var request = this.target <- Causes CPOW warning
                      .QueryInterface(Ci.nsIImageLoadingContent)
                      .getRequest(Ci.nsIImageLoadingContent.CURRENT_REQUEST);
    if (!request)
      return true;

    return false;
  },
  
  setDesktopBackground: function() {
    // Paranoia: check disableSetDesktopBackground again, in case the
    // image changed since the context menu was initiated.
    if (this.disableSetDesktopBackground())
      return;

    var doc = this.target.ownerDocument; <- Causes CPOW warning
    urlSecurityCheck(this.target.currentURI.spec, this.principal); <- Causes CPOW warning

    // Confirm since it's annoying if you hit this accidentally.
    const kDesktopBackgroundURL = 
                  "chrome://browser/content/setDesktopBackground.xul";
    // On non-Mac platforms, the Set Wallpaper dialog is modal.
    openDialog(kDesktopBackgroundURL, "",
               "centerscreen,chrome,dialog,modal,dependent",
               this.target);
  },



in browser/components/shell/content/setDesktopBackground.js

"TypeError: Argument 1 of CanvasRenderingContext2D.drawImage could not be converted to any of: HTMLImageElement, HTMLCanvasElement, HTMLVideoElement."
or in the TILE case
"TypeError: Argument 1 of CanvasRenderingContext2D.createPattern could not be converted to any of: HTMLImageElement, HTMLCanvasElement, HTMLVideoElement."

  updatePosition: function ()
  {
    var ctx = this._canvas.getContext("2d");
    ctx.clearRect(0, 0, this._screenWidth, this._screenHeight);

    this._position = document.getElementById("menuPosition").value;

    switch (this._position) {
      case "TILE":
        ctx.save();
        ctx.fillStyle = ctx.createPattern(this._image, "repeat"); <- Causes TypeError
        ctx.fillRect(0, 0, this._screenWidth, this._screenHeight);
        ctx.restore();
        break;
      case "STRETCH":
        ctx.drawImage(this._image, 0, 0, this._screenWidth, this._screenHeight); <- Causes TypeError
        break;
      case "CENTER":
        var x = (this._screenWidth - this._image.naturalWidth) / 2; <- Causes CPOW warning
        var y = (this._screenHeight - this._image.naturalHeight) / 2; <- Causes CPOW warning
        ctx.drawImage(this._image, x, y); <- Causes TypeError
        break;
      case "FILL":
        //Try maxing width first, overflow height
        var widthRatio = this._screenWidth / this._image.naturalWidth; <- Causes CPOW warning
        var width = this._image.naturalWidth * widthRatio; <- Causes CPOW warning
        var height = this._image.naturalHeight * widthRatio; <- Causes CPOW warning
        if (height < this._screenHeight) {
          //height less than screen, max height and overflow width
          var heightRatio = this._screenHeight / this._image.naturalHeight; <- Causes CPOW warning
          width = this._image.naturalWidth * heightRatio; <- Causes CPOW warning
          height = this._image.naturalHeight * heightRatio; <- Causes CPOW warning
        }
        var x = (this._screenWidth - width) / 2;
        var y = (this._screenHeight - height) / 2;
        ctx.drawImage(this._image, x, y, width, height); <- Causes TypeError
        break;
      case "FIT":
        //Try maxing width first, top and bottom borders
        var widthRatio = this._screenWidth / this._image.naturalWidth; <- Causes CPOW warning
        var width = this._image.naturalWidth * widthRatio; <- Causes CPOW warning
        var height = this._image.naturalHeight * widthRatio; <- Causes CPOW warning
        var x = 0;
        var y = (this._screenHeight - height) / 2;
        if (height > this._screenHeight) {
          //height overflow, maximise height, side borders
          var heightRatio = this._screenHeight / this._image.naturalHeight; <- Causes CPOW warning
          width = this._image.naturalWidth * heightRatio; <- Causes CPOW warning
          height = this._image.naturalHeight * heightRatio; <- Causes CPOW warning
          x = (this._screenWidth - width) / 2;
          y = 0;
        }
        ctx.drawImage(this._image, x, y, width, height); <- Causes TypeError
        break;      
    }
  }

Actually hitting "Set Desktop Background" in the preview dialog fails with "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsIShellService.setDesktopBackground]"

  setDesktopBackground: function ()
  {
    document.persist("menuPosition", "value");
    this._shell.desktopBackgroundColor = this._hexStringToLong(this._backgroundColor);
    this._shell.setDesktopBackground(this._image, <- Causes NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE
                                     Ci.nsIShellService["BACKGROUND_" + this._position]);
  },

Comment 1

[Ian Moody [:Kwan] (UTC+1)]

Attachment: MozReview Request: bz://1135933/Kwan

/r/7097 - Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image URL and make a new element with it to pass to the setDesktopBackground dialog

Pull down this commit:

hg pull -r 1b17b7476a88314a3884b2ea36186ea39c926523 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/7095/#review5909

::: browser/base/content/content.js
(Diff revision 1)
> +function disableSetDesktopBackground(aTarget) {

This duplication of the function from nsContextMenu is unfortunate, but I don't know of a better solution.

::: browser/base/content/content.js
(Diff revision 1)
> +      Cu.reportError(e);

Don't know if this is necessary.

Comment 3

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8593176 [details]
MozReview Request: bz://1135933/Kwan

Clearing review since after some discussion with jimm on IRC it seems this might not be the best approach security-wise, as I'd feared.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Actually I don't see any easy way to do this right at first glance. The problem is that the original version just passed the content image to the dialog window with the arguments property, and then the dialog window uses it for various things a CPOW could not be ever used for (http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/content/setDesktopBackground.js#167)

Now the dialog window if I assume it right runs in the parent process (because of xul) where we don't want to load an image from some random content URI, I'm not sure what we should do here...

I think even if dialog window had remote support (have they?) it can be hard to open a dialog window whose child runs in the same process as the content (or do we have that magic?)... If we could that would be a fix for this, but then we still needed to find a way to pass this image argument... Eh... Have we faced any similar issue before? Am I missing an obvious solution here?

Comment 5

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Actually I don't see any easy way to do this right at first glance. The
> problem is that the original version just passed the content image to the
> dialog window with the arguments property, and then the dialog window uses
> it for various things a CPOW could not be ever used for
> (http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/
> content/setDesktopBackground.js#167)
> 
> Now the dialog window if I assume it right runs in the parent process
> (because of xul) where we don't want to load an image from some random
> content URI, I'm not sure what we should do here...
> 
> I think even if dialog window had remote support (have they?) it can be hard
> to open a dialog window whose child runs in the same process as the content
> (or do we have that magic?)...

At the browser chrome level, no, we do not yet have that magic. We're definitely going to need that magic once was start supporting dom.ipc.processCount > 1.

In the meantime, however, we're OK assuming that the content process is being shared.

> If we could that would be a fix for this, but
> then we still needed to find a way to pass this image argument... Eh... Have
> we faced any similar issue before? Am I missing an obvious solution here?

Would it be advisable to convert the image from web content into a data URI, and then pass that to the XUL dialog instead?

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

jimm advised getting some advice from sstamm about the security aspects.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> Would it be advisable to convert the image from web content into a data URI,
> and then pass that to the XUL dialog instead?

I like the idea. We would still have to copy all the attributes probably and I hope we
can just ignore styles...

Comment 8

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> > Would it be advisable to convert the image from web content into a data URI,
> > and then pass that to the XUL dialog instead?
> 
> I like the idea. We would still have to copy all the attributes probably and
> I hope we
> can just ignore styles...
I don't think any of the attributes matter, do they?  All that's needed is the naturalWidth/Height properties, and they can be used to set the dimensions of the canvas the image is drawn to and then toDataURL()ed from (or does Mozilla have a quicker way of converting an image?).  Using that approach building on top of my previous patch I've already got it up and working.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Ian Moody [:Kwan] from comment #8)
> I don't think any of the attributes matter, do they?  All that's needed is
> the naturalWidth/Height properties

You're right, that's all what we need indeed.

> (or does Mozilla have a quicker way of converting an image?). 

Not that I'm aware of. Also, I wouldn't worry too much about
the performance of the conversion here anyway...

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

Attachment: MozReview Request: bz://1135933/Kwan

/r/7333 - Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog

Pull down this commit:

hg pull -r e92df83356c301cac7295983881d7491d3b4772e https://reviewboard-hg.mozilla.org/gecko/

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> (In reply to Ian Moody [:Kwan] from comment #8)
> > (or does Mozilla have a quicker way of converting an image?). 
> 
> Not that I'm aware of. Also, I wouldn't worry too much about
> the performance of the conversion here anyway...
Heh, I phrased it poorly, but I was referring to the code verbosity of creating a canvas and drawing an image to it, as opposed to a toDataURL() directly on the image.

Here's the patch anyway.  Question for sstamm now is is this safe security wise, or is it still susceptible to imagelib vulnerabilities in the parent process?

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Ian Moody [:Kwan] from comment #11)
> Here's the patch anyway.  Question for sstamm now is is this safe security
> wise, or is it still susceptible to imagelib vulnerabilities in the parent
> process?

I'm afraid I don't have enough background (or time) here to be super-helpful without some context.  What specifically are you worried about?

Comment 13

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12)
> (In reply to Ian Moody [:Kwan] from comment #11)
> > Here's the patch anyway.  Question for sstamm now is is this safe security
> > wise, or is it still susceptible to imagelib vulnerabilities in the parent
> > process?
> 
> I'm afraid I don't have enough background (or time) here to be super-helpful
> without some context.  What specifically are you worried about?
Ah, sorry for the lack of context.

So currently to set the background image the image element itself is passed.  That isn't safe in e10s since the <img/> is in a separate process.
My first approach for fixing this was to pass up the image URL to the parent process and create a new image element there and set its src.  However jimm informed me on IRC that this isn't safe, since if the image triggers a vulnerability in the image lib it'll be in the parent process with greater privileges.
The new approach, suggested by mconley, is to draw the image to a <canvas/>, then convert that <canvas/> to a data URL, and pass that up to the parent process and create a new <img> with said data URL as its src.  The question is is that safe (since the passed data has been created by Firefox based on a <canvas/>), or is it still susceptible to imglib vulnerabilities?
If it's not safe, do you have any other suggestions for safely getting image data based on a web image into the parent process and using it?  Trying to set the background image in the content process doesn't seem like it's viable, since I tink that'll be outside its privileges, and there is also the issue of the preview canvas.

Comment 14

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

Not seeing any reviewers on this stuff - Ian, is this ready for review?

Comment 15

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #14)
> Not seeing any reviewers on this stuff - Ian, is this ready for review?
I probably should have needinfo'd Sid on comment 13, to see if he has an opinion on if this is safe.  If it is then it's good to go for review.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

I'm not super familiar with how canvas is created and what kinds of protections between imglib and the parent process might exist, or what the differences are in our image decoding.  I've sent out a request for more feedback on this bug from a wider group of security folks.

But with that disclaimer, let me see if I understand this right:
* in the child process we use the "risky" decoding routine in imglib to load and convert the image from URL to some base64-encoded data: URI thing that doesn't require using imglib for decoding or rendering.
* we pass the data: URI to the parent
* the parent decodes and renders the data: URI (not using imglib).
* We trust the code that does any decoding of a data: URI image (not using imglib).

Is this correct?  If we only use imglib in the child, the parent should be safe from imglib problems.  

(On the other hand, if we do any calls into imglib in the parent, then we just moved the problems around.)

Comment 17

[Seth Fowler [:seth] [:s2h]]

(In reply to Ian Moody [:Kwan] from comment #13)
> The new approach, suggested by mconley, is to draw the image to a <canvas/>,
> then convert that <canvas/> to a data URL, and pass that up to the parent
> process and create a new <img> with said data URL as its src.  The question
> is is that safe (since the passed data has been created by Firefox based on
> a <canvas/>), or is it still susceptible to imglib vulnerabilities?

If the goal is to avoid invoking ImageLib, this doesn't achieve that goal. You're taking a different Necko code path, but the ImageLib code that gets executed is exactly the same.

However, since you're re-encoding the image, at least you know that it has been encoded by our own encoder, rather than a potentially malicious encoder. So there is definitely a gain in safety.

I also don't know of a better approach that works from JS. =\ (And I generally don't know much about E10S.)

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #16)
> * in the child process we use the "risky" decoding routine in imglib to load
> and convert the image from URL to some base64-encoded data: URI thing that
> doesn't require using imglib for decoding or rendering.
> * we pass the data: URI to the parent
> * the parent decodes and renders the data: URI (not using imglib).
> * We trust the code that does any decoding of a data: URI image (not using
> imglib).

Just to be sure I've been clear: this is incorrect. ImageLib is used to decode the data URI in the parent.

Comment 18

[Daniel Veditz [:dveditz]]

Do we need to decode it again in the parent? If we're setting the desktop background don't we just pass the image itself to the OS and let the OS decode it? (That, too, carries risk.)

Either way, if we have re-encoded the image ourselves from a canvas I'm much less worried about this being exploitable.

Comment 19

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Seth Fowler [:seth] from comment #17)> 
> However, since you're re-encoding the image, at least you know that it has
> been encoded by our own encoder, rather than a potentially malicious
> encoder. So there is definitely a gain in safety
Yes, this was the goal of this approach.

(In reply to Daniel Veditz [:dveditz] from comment #18)
> Do we need to decode it again in the parent? If we're setting the desktop
> background don't we just pass the image itself to the OS and let the OS
> decode it? (That, too, carries risk.)
We can't pass the image itself to the shell service when the shell service is invoked in the parent since the native code can't take a CPOW (hence the NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE), and I don't believe setting it from the content process will be allowed due to the reduced privileges it will have, so I think we do need an image element in the parent process.

Comment 20

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8594879 [details]
MozReview Request: bz://1135933/Kwan

/r/9365 - Bug 1135933 - [Review only commit] Move nsContextMenu.disableSetDesktopBackground() to content.js
/r/7333 - Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog

Pull down these commits:

hg pull -r 3a69c0ec3a2a8e90ea851632a9cece442107831e https://reviewboard-hg.mozilla.org/gecko/

Comment 21

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/7333/#review8317

Looks nice!

::: browser/base/content/content.js:692
(Diff revision 2)
> +  let disable = false;
> +
> +  // Paranoia: check disableSetDesktopBackground again, in case the
> +  // image changed since the context menu was initiated.
> +  if (disableSetDesktopBackground(target)) {
> +    disable = true;
> +  } else {
> +    try {

How about:
let disabled = disableSetDesktopBackground(target);
if (!disabled) {
  try ...
}

Also I kind of find disableSetDesktopBackground a missleading name, since it does not seem to disable anything... isSetDesktopBackgroundDisabled would make more sense to me, but that's kind of unrelated to this patch so nevermind.

::: browser/base/content/nsContextMenu.js:1164
(Diff revision 2)
> -    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> -                       .getService(Components.interfaces.nsIWindowMediator);
> +      const wm = Cc["@mozilla.org/appshell/window-mediator;1"].
> +                 getService(Ci.nsIWindowMediator);

I think the . and the indentation is off here.

Comment 22

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/9365/#review8321

Thanks for this part!

Comment 23

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/7333/#review8363

> How about:
> let disabled = disableSetDesktopBackground(target);
> if (!disabled) {
>   try ...
> }
> 
> Also I kind of find disableSetDesktopBackground a missleading name, since it does not seem to disable anything... isSetDesktopBackgroundDisabled would make more sense to me, but that's kind of unrelated to this patch so nevermind.

Ah, that's nice and concise, thanks (though I stuck with "disable" rather than "disabled").

> I think the . and the indentation is off here.

I agree it's weird, but it's also the style used consistently (!) for Cc[] throughout the rest of nsContextMenu.js, so I'm going to leave as is.

Comment 24

[Ian Moody [:Kwan] (UTC+1)]

Attachment: MozReview Request: Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor

Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r=gabor

Also perform the disableSetDesktopBackground() check in content

Comment 25

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8612541 [details]
MozReview Request: Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor

Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r=gabor

Also perform the disableSetDesktopBackground() check in content

Comment 26

[Ian Moody [:Kwan] (UTC+1)]

Review-only commit folded and comment addressed, and patch rebased to tip.

Green try is at
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf4046ce42c
with two intermittents from bug 1081925 and bug 1167579
It is for the pre-review patch but given the insignificance of the change (https://reviewboard.mozilla.org/r/7331/diff/2-4/) I don't see a need for a new run.

I presume the lack of updated hash posting it to do with the switch to separate attachments for each reviewboard commit, so here it is

Pull down this commit:

hg pull -r 2755ecafebb4d83dced05dccb79d26af8b6b90ce https://reviewboard-hg.mozilla.org/gecko/

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2a114efdfc3f

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Backed out for making browser_addKeywordSearch.js permafail.
https://treeherder.mozilla.org/logviewer.html#?job_id=3265846&repo=fx-team

Comment 29

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/ca5480ad6ffb

Comment 30

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

Hey Kwan - any chance you'll have time to figure out what's going wrong in browser_addKeywordSearch.js, or would you like to hand it off?

Comment 32

[Ian Moody [:Kwan] (UTC+1)]

Attachment: MozReview Request: Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image URL and make a new element with it to pass to the setDesktopBackground dialog

Comment 33

[Ian Moody [:Kwan] (UTC+1)]

Attachment: MozReview Request: Bug 1135933 - [Review only commit] Move nsContextMenu.disableSetDesktopBackground() to content.js

Comment 34

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> Hey Kwan - any chance you'll have time to figure out what's going wrong in
> browser_addKeywordSearch.js, or would you like to hand it off?

Yes! Sorry I haven't been more responsive, unfortunately I've been lacking in free time, but I will be able to get to it tomorrow.

Comment 35

[Ian Moody [:Kwan] (UTC+1)]

So the problem is a rather simple one, and revealed by the "TypeError: aTarget.currentURI is null" in the test logs.  When I replaced the menu init call to disableSetDesktopBackground() so I could move the function to content rather than just copy it for the paranoid check, I failed to notice something rather important: It's called inside "if (haveSetDesktopBackground && this.onLoadedImage) {}", which implies a bunch of things (https://hg.mozilla.org/mozilla-central/file/e10e2e8d8bf2/browser/base/content/nsContextMenu.js#l640), namely

    if (this.target.nodeType == Node.ELEMENT_NODE) {
      if (this.target instanceof Ci.nsIImageLoadingContent &&
          this.target.currentURI) {

So when the function is called in content.js on an <input/> is fails at 'aTarget.currentURI.schemeIs("javascript")' and the menu never opens.
There were no test failures on try because it seems no test tries to open the context menu on an <input/>, but when I rewrote browser_addKeywordSearch.js I made it do so, so that's why it now fails.

There is already an equivalent if check nearby for the media cache stuff, so moving the call inside that is enough to fix this, I believe.

Comment 36

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8612541 [details]
MozReview Request: Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor

Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor

Also perform the disableSetDesktopBackground() check in content

Comment 37

[Gabor Krizsanits (INACTIVE)]

(In reply to Ian Moody [:Kwan] from comment #35)
> There is already an equivalent if check nearby for the media cache stuff, so
> moving the call inside that is enough to fix this, I believe.

Thanks for looking into this, I guess I should have spotted this one... Anyway, a few things I don't quite get in the fix. Why do you set the initial value to null and not false? (I can't find any difference between how those two values are handled here...)  I feel like disableSetDesktopBackground should just handle the case where currentURI is null. But do we really want to enable set bg in that case? I think currentURI is null if there is no current image request and there weren't any images loaded either so I think disableSetDesktopBackground should return true in this case, no?

Comment 38

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #37)
> (In reply to Ian Moody [:Kwan] from comment #35)
> > There is already an equivalent if check nearby for the media cache stuff, so
> > moving the call inside that is enough to fix this, I believe.
> 
> Thanks for looking into this, I guess I should have spotted this one...
> Anyway, a few things I don't quite get in the fix. Why do you set the
> initial value to null and not false?
I hummed and hawed a little about which to pick.  I went for null because it felt cleaner to have a null value for non-image elements where it doesn't apply (the adjacent cache variables are initialised to null also).  And I figured the review could always result in changing it anyway if null was unsatisfactory.
> (I can't find any difference between
> how those two values are handled here...)  I feel like
> disableSetDesktopBackground should just handle the case where currentURI is
> null. But do we really want to enable set bg in that case?
I suppose it could but I was keeping the function as unchanged as possible.  Also when currentURI is null the passed-up value is never going to be used anyway, since currentURI has to exist for any of the desktop background stuff to be in the context menu (due to being contingent on nsContextMenu.onLoadedImage which implies currentURI https://hg.mozilla.org/mozilla-central/annotate/cd0d976e5f5c/browser/base/content/nsContextMenu.js#l239)

Comment 39

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8612541 [details]
MozReview Request: Bug 1135933 - Make nsContextMenu.setDesktopBackground() use a message to get the image as a dataURL and make a new element with it to pass to the setDesktopBackground dialog. r?gabor

I think your version makes just as much sense as what I suggested, so I'm fine with the current version, I don't want to hold you back with nuances just wanted to understand it better before I stamp r+ on it :)

Comment 40

[Ian Moody [:Kwan] (UTC+1)]

Green try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8808996d628

And thank you Ryan for the backout before, if you happen to be the one to see this.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b3db91f3be69

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3db91f3be69