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

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Menus
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Kwan, Assigned: Kwan)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox40 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
+++ 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.
tracking-e10s: ? → m8+
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
See Also: → bug 1141709
(Assignee)

Updated

3 years ago
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Created 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 11cc32ce8a19373889e70e9359f799ff89736825 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592295 - Flags: review?(gkrizsanits)
https://reviewboard.mozilla.org/r/7037/#review5865

Ship It!
https://reviewboard.mozilla.org/r/7039/#review5867

Ship It!
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+
(Assignee)

Comment 8

3 years ago
(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
(Assignee)

Comment 9

3 years ago
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+
(Assignee)

Comment 11

3 years ago
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/integration/fx-team/rev/bdde7ee3b6fa
https://hg.mozilla.org/integration/fx-team/rev/1eb338deaf32
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bdde7ee3b6fa
https://hg.mozilla.org/mozilla-central/rev/1eb338deaf32
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
(Assignee)

Comment 14

3 years ago
Comment on attachment 8592295 [details]
MozReview Request: bz://1140859/Kwan
Attachment #8592295 - Attachment is obsolete: true
Attachment #8619698 - Flags: review+
Attachment #8619699 - Flags: review+
(Assignee)

Comment 15

3 years ago
Created attachment 8619698 [details]
MozReview Request: Bug 1140859 - Pass the safe documentURIObject into nsContextMenu.saveHelper() rather than unsafely accessing it on the document
(Assignee)

Comment 16

3 years ago
Created attachment 8619699 [details]
MozReview Request: Bug 1140859 - Pass the outerWindowID into nsContextMenu.saveHelper() and use that to get the window, rather than unsafely accessing doc.defaultView
Note that this probably broke any of the uses of that window id in non-e10s mode.  See bug 1194764 comment 17.
Duplicate of this bug: 1141709
You need to log in before you can comment on or make changes to this bug.