1146591 - [e10s] helper_outerhtml_test_runner.js causes unsafe CPOW usage warnings

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

Mined from test logs

In browser/devtools/markupview/test/helper_outerhtml_test_runner.js

/**
 * Run a single edit-outer-html test.
 * See runEditOuterHTMLTests for a description.
 * @param {Object} test A test object should contain the following properties:
 *        - selector {String} a css selector targeting the node to edit
 *        - oldHTML {String}
 *        - newHTML {String}
 *        - validate {Function} will be executed when the edition test is done,
 *        after the new outer-html has been inserted. Should be used to verify
 *        the actual DOM, see if it corresponds to the newHTML string provided
 * @param {InspectorPanel} inspector The instance of InspectorPanel currently
 * opened
 */
function* runEditOuterHTMLTest(test, inspector) {
  info("Running an edit outerHTML test on '" + test.selector + "'");
  yield selectNode(test.selector, inspector);
  let oldNodeFront = inspector.selection.nodeFront;

  let onUpdated = inspector.once("inspector-updated");

  info("Listen for reselectedonremoved and edit the outerHTML");
  let onReselected = inspector.markup.once("reselectedonremoved");
  yield inspector.markup.updateNodeOuterHTML(inspector.selection.nodeFront,
                                             test.newHTML, test.oldHTML);
  yield onReselected;

  // Typically selectedNode will === pageNode, but if a new element has been injected in front
  // of it, this will not be the case.  If this happens.
  let selectedNodeFront = inspector.selection.nodeFront;
  let pageNodeFront = yield inspector.walker.querySelector(inspector.walker.rootNode, test.selector);
  let pageNode = getNode(test.selector);

  if (test.validate) {
    yield test.validate(pageNode, pageNodeFront, selectedNodeFront, inspector);
  } else {
    is(pageNodeFront, selectedNodeFront, "Original node (grabbed by selector) is selected");
    is(pageNode.outerHTML, test.newHTML, "Outer HTML has been updated"); <- Causes 
  }

  // Wait for the inspector to be fully updated to avoid causing errors by
  // abruptly closing hanging requests when the test ends
  yield onUpdated;
}

Comment 1

[Patrick Brosset <:pbro>]

Depends on bug 1146566 which, when resolved, will load the frame-script-utils.js frame-script into markupview tests and expose a useful devtools:test:getElementInfo which could be modified to also return the outerHTML of nodes.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1146591/pbrosset

/r/6075 - Bug 1146591 - Avoid unsafe CPOW usage in helper_outerhtml_test_runner.js; r=bgrins

Pull down this commit:

hg pull review -r 35b1994c6235475f2b295a201f464a5ff50a5555

Comment 3

[Patrick Brosset <:pbro>]

Sorry Brian, one more!
But it turns out there's a lot of CPOW usages in markupview tests, and I'm working on a bunch of them
I've noticed that event though the runEditOuterHTMLTest function in helper_outerhtml_test_runner.js doesn't itself needs access to pageNode, it passes it to the validate function.
Some of the validate functions use this CPOW to get the nextSibling for instance, and check things like textContent and nodeType.
So using the new "devtools:test:getDomElementInfo" message listener for this won't help because the nextSibling isn't an element but can be a textnode.
We can either use the WalkerFront to get this information, but we wouldn't really verify if the markupview worked because we wouldn't be testing the page, but the vision of the page that the Walker currently has, which could be wrong.
I'm not sure what kind of new frame-script listener I could add for this.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8583179 [details]
MozReview Request: bz://1146591/pbrosset

https://reviewboard.mozilla.org/r/6073/#review5029

Ship It!

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> I've noticed that event though the runEditOuterHTMLTest function in
> helper_outerhtml_test_runner.js doesn't itself needs access to pageNode, it
> passes it to the validate function.

How many validate function calls are currently directly accessing the CPOW?  If there aren't many then maybe the amount of spam generated in the test logs isn't going to be too big a deal.

> We can either use the WalkerFront to get this information, but we wouldn't
> really verify if the markupview worked because we wouldn't be testing the
> page, but the vision of the page that the Walker currently has, which could
> be wrong.

In theory, the frontend tests should be able to assume that the Walker is always correct about the page, and the server tests should verify that this is actually true.  In practice, we have more test coverage about page / walker state in the frontend.  I'd be fine with just using walker methods in these custom validation cases.

Here's another idea - what if we had a frame script to return the outerHTML of the entire document, and then had a helper function that used a DOMParser to construct a DOM in the parent process that could be queried?  It wouldn't be work for certain cases (iframes, special content types, etc) but I *think* you would get a consistent DOM representation on both sides of the frame script (we'd need to test to be sure).

Comment 6

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6830fd2b6a45

Comment 7

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/3bf5485eb14b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bf5485eb14b

Comment 10

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1146591 - Avoid unsafe CPOW usage in helper_outerhtml_test_runner.js; r=bgrins