Closed Bug 1140859 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
e10s m8+ ---
firefox40 --- fixed

People

(Reporter: Kwan, Assigned: Kwan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

+++ 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/
I'm going to spin out the crash to a different bug.
Filed bug 1141709.
Summary: [e10s] nsContextMenu.saveHelper() in remote browser causes unsafe CPOW usage warnings, crashes content process on links to non-existent domains → [e10s] nsContextMenu.saveHelper() in remote browser causes unsafe CPOW usage warnings
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Attached file MozReview Request: bz://1140859/Kwan (obsolete) —
/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/
Attachment #8592295 - Flags: review?(gkrizsanits)
Thanks, both patches look great!
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.
Attachment #8592295 - Flags: review?(gkrizsanits) → review+
(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 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/
Attachment #8592295 - Flags: review+ → review?(gkrizsanits)
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.
Attachment #8592295 - Flags: review?(gkrizsanits) → review+
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 :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bdde7ee3b6fa
https://hg.mozilla.org/mozilla-central/rev/1eb338deaf32
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Attachment #8592295 - Attachment is obsolete: true
Attachment #8619698 - Flags: review+
Attachment #8619699 - Flags: review+
Note that this probably broke any of the uses of that window id in non-e10s mode.  See bug 1194764 comment 17.
You need to log in before you can comment on or make changes to this bug.