[e10s] "Add a Keyword for this Search…" in remote browser causes unsafe CPOW usage warnings

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Kwan, Assigned: Kwan)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(e10sm8+, firefox41 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

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

STR:

1) Visit a site with a search box (like the one at the top of this page) on it in an e10s window
2) Right-click in the search box, and choose "Add a Keyword for this Search…"

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

In browser/base/content/browser.js

function GetSearchFieldBookmarkData(node) {
  var charset = node.ownerDocument.characterSet; <- Causes CPOW warning

  var formBaseURI = makeURI(node.form.baseURI, <- Causes CPOW warning
                            charset);

  var formURI = makeURI(node.form.getAttribute("action"), <- Causes CPOW warning
                        charset,
                        formBaseURI);

  var spec = formURI.spec;

  var isURLEncoded =
               (node.form.method.toUpperCase() == "POST" <- Causes CPOW warning
                && (node.form.enctype == "application/x-www-form-urlencoded" || <- Causes CPOW warning [I assume, didn't hit it]
                    node.form.enctype == "")); <- Causes CPOW warning [assumed]

  var title = gNavigatorBundle.getFormattedString("addKeywordTitleAutoFill",
                                                  [node.ownerDocument.title]); <- Causes CPOW warning
  var description = PlacesUIUtils.getDescriptionFromDocument(node.ownerDocument); <- Causes CPOW warning

[...]

  for (let el of node.form.elements) { <- Causes CPOW warning
    if (!el.type) // happens with fieldsets <- Causes CPOW warning
      continue;

    if (el == node) {
      formData.push((isURLEncoded) ? escapeNameValuePair(el.name, "%s", true) :  <- Causes CPOW warning [assumed]
                                     // Don't escape "%s", just append
                                     escapeNameValuePair(el.name, "", false) + "%s"); <- Causes CPOW warning
      continue;
    }

    let type = el.type.toLowerCase(); <- Causes CPOW warning

    if (((el instanceof HTMLInputElement && el.mozIsTextField(true)) || <- Causes CPOW warning
        type == "hidden" || type == "textarea") ||
        ((type == "checkbox" || type == "radio") && el.checked)) { <- Causes CPOW warning [assumed]
      formData.push(escapeNameValuePair(el.name, el.value, isURLEncoded)); <- Causes CPOW warning
    } else if (el instanceof HTMLSelectElement && el.selectedIndex >= 0) { <- Causes CPOW warning
      for (var j=0; j < el.options.length; j++) { <- Causes CPOW warning [assumed]
        if (el.options[j].selected) <- Causes CPOW warning [assumed]
          formData.push(escapeNameValuePair(el.name, el.options[j].value, <- Causes CPOW warning [assumed]
                                            isURLEncoded));
      }
    }
  }

[...]
}

plus I think pretty much the same bunch in PlacesUIUtils.getDescriptionFromDocument() as in bug 1135619, which I won't list right now.
Assignee

Updated

4 years ago
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Assignee

Comment 1

4 years ago
/r/8415 - Bug 1141350 - Use a message to get search field bookmark data for 'Add a Keyword for this Search...'

Pull down this commit:

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

Comment 2

4 years ago
Comment on attachment 8603315 [details]
MozReview Request: bz://1141350/Kwan

/r/8415 - Bug 1141350 - Use a message to get search field bookmark data for 'Add a Keyword for this Search...'
/r/8579 - Bug 1141350 - Fix browser_addKeywordSearch.js test to work with the new message-based approach

Pull down these commits:

hg pull -r 57827eb3cc38aeb6a4c6ca61e25c46f9d485c817 https://reviewboard-hg.mozilla.org/gecko/
Assignee

Comment 3

4 years ago
Comment on attachment 8603315 [details]
MozReview Request: bz://1141350/Kwan

/r/8415 - Bug 1141350 - Use a message to get search field bookmark data for 'Add a Keyword for this Search...'
/r/8579 - Bug 1141350 - Fix browser_addKeywordSearch.js test to work with the new message-based approach

Pull down these commits:

hg pull -r 2f695260694cdc6685572d30f89903703d6bc79e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8603315 - Flags: review?(gkrizsanits)
https://reviewboard.mozilla.org/r/8415/#review7491

::: browser/base/content/browser.js
(Diff revision 2)
> -function GetSearchFieldBookmarkData(node) {
> -  var charset = node.ownerDocument.characterSet;
> -
> -  var formBaseURI = makeURI(node.form.baseURI,
> -                            charset);
> -
> -  var formURI = makeURI(node.form.getAttribute("action"),
> -                        charset,
> -                        formBaseURI);
> -
> -  var spec = formURI.spec;
> -
> -  var isURLEncoded =
> -               (node.form.method.toUpperCase() == "POST"
> -                && (node.form.enctype == "application/x-www-form-urlencoded" ||
> -                    node.form.enctype == ""));
> -
> -  var title = gNavigatorBundle.getFormattedString("addKeywordTitleAutoFill",
> -                                                  [node.ownerDocument.title]);
> -  var description = PlacesUIUtils.getDescriptionFromDocument(node.ownerDocument);
> -
> -  var formData = [];
> -
> -  function escapeNameValuePair(aName, aValue, aIsFormUrlEncoded) {
> -    if (aIsFormUrlEncoded)
> -      return escape(aName + "=" + aValue);
> -    else
> -      return escape(aName) + "=" + escape(aValue);
> -  }
> -
> -  for (let el of node.form.elements) {
> -    if (!el.type) // happens with fieldsets
> -      continue;
> -
> -    if (el == node) {
> -      formData.push((isURLEncoded) ? escapeNameValuePair(el.name, "%s", true) :
> -                                     // Don't escape "%s", just append
> -                                     escapeNameValuePair(el.name, "", false) + "%s");
> -      continue;
> -    }
> -
> -    let type = el.type.toLowerCase();
> -
> -    if (((el instanceof HTMLInputElement && el.mozIsTextField(true)) ||

One thing you could do for the reviewer next time is to do this kind of work in two patches. The first just moves the code block around, and the second does the modifications on the code block. You will later merge the two patches but it makes reviewing a lot easier because I don't have to compare two huge blocks of code line by line.
https://reviewboard.mozilla.org/r/8579/#review7495

I'm not sure what guarantees that this test will not fail intermittently because of the context menu opening... Is there a way to guarantee that? (I can be wrong, so please explain me if I don't see something obvious...)

::: browser/base/content/test/general/browser_addKeywordSearch.js:58
(Diff revision 2)
> -    }
> +          is(message.data.spec, expected, `Bookmark spec for search field named ${fieldName} and baseURI ${baseURI} incorrect`);

The string could go to a new line I think...

::: browser/base/content/test/general/browser_addKeywordSearch.js:42
(Diff revision 2)
> -      is(data.spec, expected, "Bookmark spec for search field named " + fieldName + " and baseURI " + baseURI + " incorrect");
> -
> -      doc.body.removeChild(form);
> -    }
> -
> -    let testData = [
> -    /* baseURI, field name, expected */
> -      [ 'http://example.com/', 'q', 'http://example.com/?q=%s' ],
> -      [ 'http://example.com/new-path-here/', 'q', 'http://example.com/new-path-here/?q=%s' ],
> +        const domWindowUtils =
> +          content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                 .getInterface(Components.interfaces.nsIDOMWindowUtils);
> +        let rect = element.getBoundingClientRect();
> +        let left = rect.left + rect.width / 2;
> +        let top = rect.top + rect.height / 2;
> +        domWindowUtils.sendMouseEvent("contextmenu", left, top, 2,
> +                                      1, 0, false, 0, 0, true);
> +      });

I think I don't get this part. What guarantees us that the context window is opened before the rest of the code runs here? Could this introduce an intermittent failure?

::: browser/base/content/test/general/browser_addKeywordSearch.js:70
(Diff revision 2)
> +      yield ContentTask.spawn(browser, null, function* () {
> +        let doc = content.document;
> +        doc.body.removeChild(doc.getElementById("keyword-form"));
> +      });

Is there a reason not to use registerCleanupFunction for this?

::: browser/base/content/test/general/browser_addKeywordSearch.js:70
(Diff revision 2)
> +      yield ContentTask.spawn(browser, null, function* () {
> +        let doc = content.document;
> +        doc.body.removeChild(doc.getElementById("keyword-form"));
> +      });

Is there a reason not to use registerCleanupFunction for this?
Comment on attachment 8603315 [details]
MozReview Request: bz://1141350/Kwan

Canceling the review for now.
Attachment #8603315 - Flags: review?(gkrizsanits)
Assignee

Comment 8

4 years ago
https://reviewboard.mozilla.org/r/8415/#review8033

> One thing you could do for the reviewer next time is to do this kind of work in two patches. The first just moves the code block around, and the second does the modifications on the code block. You will later merge the two patches but it makes reviewing a lot easier because I don't have to compare two huge blocks of code line by line.

That is some excellent advice, thank you, I'll bear it in mind it in the future, and I've tested it out here since my workflow made it rather easy to insert such a commit.
Assignee

Comment 9

4 years ago
https://reviewboard.mozilla.org/r/8579/#review8035

> The string could go to a new line I think...

Done.

> Is there a reason not to use registerCleanupFunction for this?

Wouldn't that only run once, at the end of the entire test file?  The aim is to destroy the form after each keyword URL is verified and create a new one for the next URL.

> I think I don't get this part. What guarantees us that the context window is opened before the rest of the code runs here? Could this introduce an intermittent failure?

An excellent point.  There's some rather handy use of BrowserTestUtils.waitForEvent() in other tests that I've cribbed off to fix this.
Assignee

Comment 10

4 years ago
Comment on attachment 8603315 [details]
MozReview Request: bz://1141350/Kwan

/r/9341 - Bug 1141350 - [Review only commit] move GetSearchFieldBookmarkData() to from browser.js to content.js
/r/8415 - Bug 1141350 - Use a message to get search field bookmark data for 'Add a Keyword for this Search...'
/r/8579 - Bug 1141350 - Fix browser_addKeywordSearch.js test to work with the new message-based approach

Pull down these commits:

hg pull -r ae0a9e628452f3b7010c79d2b2e5a167de6dce62 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8603315 - Flags: review?(gkrizsanits)
Attachment #8603315 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8603315 [details]
MozReview Request: bz://1141350/Kwan

https://reviewboard.mozilla.org/r/8413/#review8061

All looks great! And thanks for the separated patch for the codeblock moving :)
Assignee

Comment 12

4 years ago
Comment on attachment 8603315 [details]
MozReview Request: bz://1141350/Kwan

/r/9341 - Bug 1141350 - Use a message to get search field bookmark data for 'Add a Keyword for this Search...'. r=gabor
/r/8579 - Bug 1141350 - Fix browser_addKeywordSearch.js test to work with the new message-based approach. r=gabor

Pull down these commits:

hg pull -r fe8b3264dd758f0e82010c91cf0fa7213d604836 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8603315 - Flags: review+
Assignee

Comment 13

4 years ago
Patches folded to remove the review-only commit.
Also made one var -> let change I'd missed (in the last for loop in content.js) that I don't see a need to request re-review for.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cc4926b86d4
Green with one intermittent from Bug 1118277.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/33be69ac6316
https://hg.mozilla.org/mozilla-central/rev/645cd148f91c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee

Comment 16

4 years ago
Attachment #8603315 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.