Closed Bug 1146591 Opened 5 years ago Closed 5 years ago

[e10s] helper_outerhtml_test_runner.js causes unsafe CPOW usage warnings

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(e10sm8+, firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
e10s m8+ ---
firefox39 --- fixed

People

(Reporter: Kwan, Assigned: pbro)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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;
}
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.
Depends on: 1146566
/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
Attachment #8583179 - Flags: review?(bgrinstead)
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 on attachment 8583179 [details]
MozReview Request: bz://1146591/pbrosset

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

Ship It!
Attachment #8583179 - Flags: review?(bgrinstead) → review+
(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).
https://hg.mozilla.org/mozilla-central/rev/3bf5485eb14b
Assignee: nobody → pbrosset
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Attachment #8583179 - Attachment is obsolete: true
Attachment #8619855 - Flags: review+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.