1140859 - [e10s] nsContextMenu.saveHelper() in remote browser causes unsafe CPOW usage warnings

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 links or media elements on it in an e10s window
2) Right-click on a link or media element, and choose "Save Link/Image/Audio/Video As.."

This causes some "unsafe CPOW usage" warnings in the Browser Console.

In browser/base/content/nsContextMenu.js:

  // Helper function to wait for appropriate MIME-type headers and
  // then prompt the user with a file picker
  saveHelper: function(linkURL, linkText, dialogTitle, bypassCache, doc) {

[...]

      onStartRequest: function saveLinkAs_onStartRequest(aRequest, aContext) {

        // if the timer fired, the error status will have been caused by that,
        // and we'll be restarting in onStopRequest, so no reason to notify
        // the user
        if (aRequest.status == NS_ERROR_SAVE_LINK_AS_TIMEOUT)
          return;

        timer.cancel();

        // some other error occured; notify the user...
        if (!Components.isSuccessCode(aRequest.status)) {
          try {
            const sbs = Cc["@mozilla.org/intl/stringbundle;1"].
                        getService(Ci.nsIStringBundleService);
            const bundle = sbs.createBundle(
                    "chrome://mozapps/locale/downloads/downloads.properties");

            const title = bundle.GetStringFromName("downloadErrorAlertTitle");
            const msg = bundle.GetStringFromName("downloadErrorGeneric");

            const promptSvc = Cc["@mozilla.org/embedcomp/prompt-service;1"].
                              getService(Ci.nsIPromptService);
            promptSvc.alert(doc.defaultView, title, msg); <- Causes CPOW warning/leads to crash [non-existent]
          } catch (ex) {}
          return;
        }

        let extHelperAppSvc = 
          Cc["@mozilla.org/uriloader/external-helper-app-service;1"].
          getService(Ci.nsIExternalHelperAppService);
        let channel = aRequest.QueryInterface(Ci.nsIChannel);
        this.extListener =
          extHelperAppSvc.doContent(channel.contentType, aRequest, 
                                    null, true, window);
        this.extListener.onStartRequest(aRequest, aContext);
      }, 

      onStopRequest: function saveLinkAs_onStopRequest(aRequest, aContext, 
                                                       aStatusCode) {
        if (aStatusCode == NS_ERROR_SAVE_LINK_AS_TIMEOUT) {
          // do it the old fashioned way, which will pick the best filename
          // it can without waiting.
          saveURL(linkURL, linkText, dialogTitle, bypassCache, false,
                  BrowserUtils.makeURIFromCPOW(doc.documentURIObject), doc); <- Causes CPOW warning [timeout]
        }
        if (this.extListener)
          this.extListener.onStopRequest(aRequest, aContext, aStatusCode);
      },
      
[...]

    if (channel instanceof Ci.nsIHttpChannel) {
      channel.referrer = BrowserUtils.makeURIFromCPOW(doc.documentURIObject); <- Causes CPOW warning
      if (channel instanceof Ci.nsIHttpChannelInternal)
        channel.forceAllowThirdPartyCookie = true;
    }

    // fallback to the old way if we don't see the headers quickly 
    var timeToWait = 
      gPrefService.getIntPref("browser.download.saveLinkAsFilenameTimeout");
    var timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
    timer.initWithCallback(new timerCallback(), timeToWait,
                           timer.TYPE_ONE_SHOT);

    // kick off the channel with our proxy object as the listener
    channel.asyncOpen(new saveAsListener(), null);
  },

and in the crashing case we get:
Error: child process crashed or timedout nsPrompter.js:356:24

in toolkit/components/prompts/src/nsPrompter.js:

function openModalWindow(domWin, uri, args) {
    // There's an implied contract that says modal prompts should still work
    // when no "parent" window is passed for the dialog (eg, the "Master
    // Password" dialog does this).  These prompts must be shown even if there
    // are *no* visible windows at all.
    // There's also a requirement for prompts to be blocked if a window is
    // passed and that window is hidden (eg, auth prompts are supressed if the
    // passed window is the hidden window).
    // See bug 875157 comment 30 for more...
    if (domWin) {
        // a domWin was passed, so we can apply the check for it being hidden.
        let winUtils = domWin.QueryInterface(Ci.nsIInterfaceRequestor) <- Causes CPOW warning
                             .getInterface(Ci.nsIDOMWindowUtils);

        if (winUtils && !winUtils.isParentWindowMainWidgetVisible) { <- Causes CPOW warning, crash
            throw Components.Exception("Cannot call openModalWindow on a hidden window",
                                       Cr.NS_ERROR_NOT_AVAILABLE);
        }
    } else {
        // We try and find a window to use as the parent, but don't consider
        // if that is visible before showing the prompt.
        domWin = Services.ww.activeWindow;
        // domWin may still be null here if there are _no_ windows open.
    }
    // Note that we don't need to fire DOMWillOpenModalDialog and
    // DOMModalDialogClosed events here, wwatcher's OpenWindowInternal
    // will do that. Similarly for enterModalState / leaveModalState.

    Services.ww.openWindow(domWin, uri, "_blank", "centerscreen,chrome,modal,titlebar", args);
}

The plain warnings I already have an idea/patch for how to fix.  The crash I do not.
Be aware that it seems as though sometimes it will stop crashing and instead open the save dialog.  Restoring the crash requires an unknown combination of clearing the browser cache, clearing the (host machine's) DNS resolver cache, and restarting the browser.

Some example domains to trigger the crash:
http://www.kgflkasfbe.com/
http://www.xnxnxbdxkab.com/
http://www.zxqxzxuqxzxqux.com/

Comment 1

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

I'm going to spin out the crash to a different bug.

Comment 2

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

Filed bug 1141709.

Comment 3

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

Attachment: MozReview Request: bz://1140859/Kwan

/r/7037 - Bug 1140859 - Pass the safe documentURIObject into nsContextMenu.saveHelper() rather than unsafely accessing it on the document
/r/7039 - Bug 1140859 - Pass the outerWindowID into nsContextMenu.saveHelper() and use that to get the window, rather than unsafely accessing doc.defaultView

Pull down these commits:

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

Comment 4

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/7037/#review5865

Ship It!

Comment 5

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/7039/#review5867

Ship It!

Comment 6

[Gabor Krizsanits (INACTIVE)]

Thanks, both patches look great!

Comment 7

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8592295 [details]
MozReview Request: bz://1140859/Kwan

Hmm... it seems like I need to flip the r+ flag here as well, sorry for the bug spam.

Comment 8

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> Thanks, both patches look great!
Thanks for the review!

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> Comment on attachment 8592295 [details]
> MozReview Request: bz://1140859/Kwan
> 
> Hmm... it seems like I need to flip the r+ flag here as well, sorry for the
> bug spam.
I believe I've seen this spoken of before, I think the root review request needs a "Ship It!" for auto r+ to happen.
You can test it if you want since Try just pointed out a silly capitalisation typo of gContextmenuContentData instead of gContextMenuContextData.  Clearly I failed to test these changes with saving of media as well as just links.

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

Comment 9

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

Comment on attachment 8592295 [details]
MozReview Request: bz://1140859/Kwan

/r/7037 - Bug 1140859 - Pass the safe documentURIObject into nsContextMenu.saveHelper() rather than unsafely accessing it on the document
/r/7039 - Bug 1140859 - Pass the outerWindowID into nsContextMenu.saveHelper() and use that to get the window, rather than unsafely accessing doc.defaultView

Pull down these commits:

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

Comment 10

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8592295 [details]
MozReview Request: bz://1140859/Kwan

I don't think you need to request another review in case of a typo, or other trivial mistake in general that try discovers... it happens :) Sorry for the lag with the re-review I just had a busy day yesterday.

Comment 11

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

Green try in comment #8.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)
> Comment on attachment 8592295 [details]
> MozReview Request: bz://1140859/Kwan
> 
> I don't think you need to request another review in case of a typo, or other
> trivial mistake in general that try discovers... it happens :) Sorry for the
> lag with the re-review I just had a busy day yesterday.
Ah, I did wonder, but decided to err on the side of caution. Thanks for letting me know, I'll remember that in the future.
And no worries about the lag :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/bdde7ee3b6fa
https://hg.mozilla.org/integration/fx-team/rev/1eb338deaf32

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bdde7ee3b6fa
https://hg.mozilla.org/mozilla-central/rev/1eb338deaf32

Comment 15

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

Attachment: MozReview Request: Bug 1140859 - Pass the safe documentURIObject into nsContextMenu.saveHelper() rather than unsafely accessing it on the document

Comment 16

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

Attachment: MozReview Request: Bug 1140859 - Pass the outerWindowID into nsContextMenu.saveHelper() and use that to get the window, rather than unsafely accessing doc.defaultView

Comment 17

[Boris Zbarsky [:bzbarsky]]

Note that this probably broke any of the uses of that window id in non-e10s mode.  See bug 1194764 comment 17.