1170291 - Unsafe CPOW usage in feeds.js, permission.js, security.js for pageInfo

Status: RESOLVED DUPLICATE of bug 1040947

(Firefox :: Page Info Window, defect)

Comment 1

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: fix-feeds-security-permission-unsafe-cpow

Comment 2

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: fix-feeds-security-permission-unsafe-cpow

Patch done on top of patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1040947

Removes Unsafe CPOW operations for feeds, security and permissions tab.
Try server is broken at the moment. Will push to try to test later.
Review can be done before that though.

Comment 3

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: bug-1170291-remove-security-permission-feeds-unsafe-cpow

Removes unsafe CPOW operations from feeds.js, security.js, permissions.js
Remove use of gWindow (CPOW wrapper window) and replace it with a JSON gWindowInfo object

Comment 4

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: bug-1170291-remove-security-permission-feeds-unsafe-cpow

Patch on top of https://bugzilla.mozilla.org/show_bug.cgi?id=1040947

Fixed failing test before.
Remove the use of onImagePreviewShown in pageInfo.js.
All tests now use onFinished to push their tests to run after the pageInfo info has been loaded.

Remove use of unsafe CPOW for feeds, permissions, security

Comment 5

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: bug-1170291-remove-security-permission-feeds-unsafe-cpow

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8621101 [details] [diff] [review]
bug-1170291-remove-security-permission-feeds-unsafe-cpow

Review of attachment 8621101 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +38,5 @@
>    Cu.import("resource://gre/modules/PageMenu.jsm", tmp);
>    return new tmp.PageMenuChild();
>  });
>  
> +//for pageInfoListener module for dealing with page rss feeds

This comment isn't clear. Either say something like "For the Feeds tab." or just remove it, as it's kinda obvious which tab is using this :).

@@ +808,5 @@
> +      hName = this.window.location.host;
> +    }
> +    catch (exception) { }
> +
> +    windowInfo.hName = hName;

hName isn't a meaningful name to me. I know this name existed before, but this is probably a good opportunity to change it.

@@ +825,5 @@
> +
> +    let documentURIObject = {};
> +    documentURIObject.spec = this.document.documentURIObject.spec;
> +    documentURIObject.originCharset = this.document.documentURIObject.originCharset;
> +    docInfo.documentURIObject = documentURIObject;

Can we transfer URI objects directly? If not, could we reconstruct it in the parent process so that it behaves like a normal URI object?

@@ +852,5 @@
> +
> +      if (rels.feed || (link.type && rels.alternate && !rels.stylesheet)) {
> +        var type = Feeds.isValidFeed(link, this.document.nodePrincipal, "feed" in rels);
> +        if (type) {
> +           type = this.feedTypes[type] || this.feedTypes["application/rss+xml"];

nit: indent should be 2 spaces here, not 3.

@@ +905,1 @@
>     * Then do setTimeOut so the UI doesn't hang and the function was continue to fetch media elements.

setTimeout, not setTimeOut.

@@ +905,3 @@
>     * Then do setTimeOut so the UI doesn't hang and the function was continue to fetch media elements.
>     * We sendAsyncMessage after it processes all the elements. So setTimeOut doesn't do anything.
>     * #TODO: refactor pageInfo.js to receive 500 media elements at a time from messages and continually update UI

I don't think 500 elements at a time would be really useful, most pages don't have 500 different media items. How expensive would it be to send a message for each media item?

::: browser/base/content/pageinfo/pageInfo.js
@@ +305,5 @@
>  // changes. For example, at this time, the list of images of the Media
>  // tab is cleared.
>  var onResetRegistry = [ ];
>  
> +// Used by tests to who push functions to execute after the child process sends back data to load the pageInfo

The previous comment was better. This has been added primarily for add-on authors. Tests just happen to also use it :).

@@ +340,5 @@
>               window.arguments.length >= 1 &&
>               window.arguments[0];
>  
>    if (!args || !args.doc) {
> +    gDocument = window.opener.gBrowser.selectedBrowser.contentWindowAsCPOW.document;

Why is this still needed? I thought the goal of this second patch was to get rid of the remaining CPOWs?

@@ +370,3 @@
>    //look for pageInfoListener in content.js. Sends message to listener with gString as an argument
> +  mm.sendAsyncMessage("PageInfo:getData",
> +                      {gStrings: gStrings,frameOuterWindowID: frameOuterWindowID, feedTypes: feedTypes});

nit: missing space after the first comma.

@@ +380,2 @@
>  
> +    //gWindowInfo.isEqualToWindowTop is for the content window. window != window.top

isEqualToWindowTop should be isTopWindow

@@ +393,3 @@
>  
> +    // Loop through onFinished and execute the functions on it.
> +    onFinished.forEach(function(func) { func(); });

I see you've already done here something I suggested in my review in bug 1040947 :).

::: browser/base/content/pageinfo/permissions.js
@@ +29,5 @@
>  };
>  
>  function onLoadPermission()
>  {
> +  var uri = BrowserUtils.makeURI(gDocInfo.documentURIObject.spec, gDocInfo.documentURIObject.originCharset, null);

don't you just want:
  var uri = gDocInfo.documentURIObject;
?

::: browser/base/content/pageinfo/security.js
@@ +20,5 @@
>      const nsISSLStatus = Components.interfaces.nsISSLStatus;
>  
>      // We don't have separate info for a frame, return null until further notice
>      // (see bug 138479)
> +    //gWindowInfo.isEqualToWindowTop is for the content window. window != window.top

This comment doesn't make sense, and I'm not sure you need it.

@@ +133,5 @@
>      var eTLDService = Components.classes["@mozilla.org/network/effective-tld-service;1"].
>                        getService(Components.interfaces.nsIEffectiveTLDService);
>  
>      var eTLD;
> +    var uri = BrowserUtils.makeURI(gDocInfo.documentURIObject.spec, gDocInfo.documentURIObject.originCharset, null);

same comment here (and at the next few occurrences), isn't gDocInfo.documentURIObject enough?

::: browser/base/content/test/general/browser_bug517902.js
@@ +13,5 @@
>      var pageInfo = BrowserPageInfo(doc, "mediaTab", testImg);
>  
>      pageInfo.addEventListener("load", function () {
>        pageInfo.removeEventListener("load", arguments.callee, true);
> +      pageInfo.onFinished.push(function () {

Is changing from onImagePreviewShown to onFinished really OK here?

::: browser/base/content/test/plugins/browser_pageInfo_plugins.js
@@ +33,5 @@
>  }
>  
>  function pageInfoObserve(win, topic, data) {
>    Services.obs.removeObserver(pageInfoObserve, "page-info-dialog-loaded");
> +  gPageInfo.onFinished.push( () => executeSoon(gNextTest));

nit: remove the space before ().