Bug 1405018 - Consider current browsing context on staleness check. r?whimboo draft
authorAndreas Tolfsen <ato@sny.no>
Mon, 02 Oct 2017 19:09:26 +0100
changeset 677290 4f2019561e5ae738e87b745721aa4df30b613843
parent 677289 70d57db6065d58f52ea6a59033e062053960fd97
child 677291 64682b7d3fbbced4c2b9c63b2852109e56dadcbb
push id83750
push userbmo:ato@sny.no
push dateTue, 10 Oct 2017 14:01:34 +0000
reviewerswhimboo
bugs1405018
milestone58.0a1
Bug 1405018 - Consider current browsing context on staleness check. r?whimboo The element.isStale function does not take into account the current browsing context when checking an element’s staleness. This means, for example, that an element in an <iframe> that gets retrieved, will still be considered valid for as long as its associated document lives. In WebDriver the expected behaviour is for the element reference to only be valid for the current browsing context, meaning retrieving the element reference when another browsing context is chosen should return a stale element error. Fixes: https://github.com/mozilla/geckodriver/issues/934 MozReview-Commit-ID: JpQVt78u5AN
testing/marionette/action.js
testing/marionette/element.js
testing/marionette/evaluate.js
testing/marionette/listener.js
--- a/testing/marionette/action.js
+++ b/testing/marionette/action.js
@@ -1316,17 +1316,17 @@ function dispatchPointerMove(a, inputSta
   // interval between pointermove increments in ms, based on common vsync
   const fps60 = 17;
 
   return new Promise(resolve => {
     const start = Date.now();
     const [startX, startY] = [inputState.x, inputState.y];
 
     let target = action.computePointerDestination(a, inputState,
-        getElementCenter(a.origin, seenEls));
+        getElementCenter(a.origin, seenEls, window));
     const [targetX, targetY] = [target.x, target.y];
 
     if (!inViewPort(targetX, targetY, window)) {
       throw new MoveTargetOutOfBoundsError(
           `(${targetX}, ${targetY}) is out of bounds of viewport ` +
           `width (${window.innerWidth}) ` +
           `and height (${window.innerHeight})`);
     }
@@ -1425,17 +1425,17 @@ function capitalize(str) {
 
 function inViewPort(x, y, win) {
   assert.number(x, `Expected x to be finite number`);
   assert.number(y, `Expected y to be finite number`);
   // Viewport includes scrollbars if rendered.
   return !(x < 0 || y < 0 || x > win.innerWidth || y > win.innerHeight);
 }
 
-function getElementCenter(elementReference, seenEls) {
+function getElementCenter(elementReference, seenEls, window) {
   if (element.isWebElementReference(elementReference)) {
     let uuid = elementReference[element.Key] ||
         elementReference[element.LegacyKey];
-    let el = seenEls.get(uuid);
+    let el = seenEls.get(uuid, window);
     return element.coordinates(el);
   }
   return {};
 }
--- a/testing/marionette/element.js
+++ b/testing/marionette/element.js
@@ -130,61 +130,71 @@ element.Store = class {
     this.els[id] = Cu.getWeakReference(el);
     return id;
   }
 
   /**
    * Determine if the provided web element reference has been seen
    * before/is in the element store.
    *
+   * Unlike when getting the element, a staleness check is not
+   * performed.
+   *
    * @param {string} uuid
    *     Element's associated web element reference.
    *
    * @return {boolean}
    *     True if element is in the store, false otherwise.
    */
   has(uuid) {
     return Object.keys(this.els).includes(uuid);
   }
 
   /**
    * Retrieve a DOM element by its unique web element reference/UUID.
    *
+   * Upon retrieving the element, an element staleness check is
+   * performed.
+   *
    * @param {string} uuid
    *     Web element reference, or UUID.
+   * @param {WindowProxy} window
+   *     Current browsing context, which may differ from the associate
+   *     browsing context of <var>el</var>.
    *
    * @returns {Element}
    *     Element associated with reference.
    *
    * @throws {NoSuchElementError}
    *     If the web element reference <var>uuid</var> has not been
    *     seen before.
    * @throws {StaleElementReferenceError}
    *     If the element has gone stale, indicating it is no longer
    *     attached to the DOM, or its node document is no longer the
    *     active document.
    */
-  get(uuid) {
+  get(uuid, window) {
     if (!this.has(uuid)) {
       throw new NoSuchElementError(
           "Web element reference not seen before: " + uuid);
     }
 
     let el;
     let ref = this.els[uuid];
     try {
       el = ref.get();
     } catch (e) {
       delete this.els[uuid];
     }
 
-    if (element.isStale(el)) {
+    if (element.isStale(el, window)) {
       throw new StaleElementReferenceError(
           pprint`The element reference of ${el || uuid} stale; ` +
-              "either the element is no longer attached to the DOM " +
+              "either the element is no longer attached to the DOM, " +
+              "it is not in the current frame context, " +
               "or the document has been refreshed");
     }
 
     return el;
   }
 };
 
 /**
@@ -617,32 +627,46 @@ element.generateUUID = function() {
   let uuid = uuidGen.generateUUID().toString();
   return uuid.substring(1, uuid.length - 1);
 };
 
 /**
  * Determines if <var>el</var> is stale.
  *
  * A stale element is an element no longer attached to the DOM or which
- * node document is not the active document.
+ * node document is not the active document of the current browsing
+ * context.
  *
- * @param {Element} el
- *     DOM element to check for staleness.
+ * The currently selected browsing context, specified through
+ * <var>window<var>, is a WebDriver concept defining the target
+ * against which commands will run.  As the current browsing context
+ * may differ from <var>el</var>'s associated context, an element is
+ * considered stale even if it is connected to a living (not discarded)
+ * browsing context such as an <tt>&lt;iframe&gt;</tt>.
+ *
+ * @param {Element=} el
+ *     DOM element to check for staleness.  If null, which may be
+ *     the case if the element has been unwrapped from a weak
+ *     reference, it is always considered stale.
+ * @param {WindowProxy=} window
+ *     Current browsing context, which may differ from the associate
+ *     browsing context of <var>el</var>.  When retrieving XUL
+ *     elements, this is optional.
  *
  * @return {boolean}
  *     True if <var>el</var> is stale, false otherwise.
  */
-element.isStale = function(el) {
-  if (!el) {
-    return true;
+element.isStale = function(el, window = undefined) {
+  if (typeof window == "undefined") {
+    window = el.ownerGlobal;
   }
 
-  let doc = el.ownerDocument;
-  let win = doc.defaultView;
-  if (!win || el.ownerDocument !== win.document) {
+  if (el === null ||
+      !el.ownerGlobal ||
+      el.ownerDocument !== window.document) {
     return true;
   }
 
   return !el.isConnected;
 };
 
 /**
  * This function generates a pair of coordinates relative to the viewport
--- a/testing/marionette/evaluate.js
+++ b/testing/marionette/evaluate.js
@@ -185,59 +185,57 @@ evaluate.sandbox = function(sb, script, 
 /**
  * Convert any web elements in arbitrary objects to DOM elements by
  * looking them up in the seen element store.
  *
  * @param {Object} obj
  *     Arbitrary object containing web elements.
  * @param {element.Store} seenEls
  *     Element store to use for lookup of web element references.
- * @param {Window} win
- *     Window.
- * @param {ShadowRoot} shadowRoot
- *     Shadow root.
+ * @param {WindowProxy} window
+ *     Current browsing context.
  *
  * @return {Object}
  *     Same object as provided by <var>obj</var> with the web elements
  *     replaced by DOM elements.
  */
-evaluate.fromJSON = function(obj, seenEls, win, shadowRoot = undefined) {
+evaluate.fromJSON = function(obj, seenEls, window) {
   switch (typeof obj) {
     case "boolean":
     case "number":
     case "string":
     default:
       return obj;
 
     case "object":
       if (obj === null) {
         return obj;
 
       // arrays
       } else if (Array.isArray(obj)) {
-        return obj.map(e => evaluate.fromJSON(e, seenEls, win, shadowRoot));
+        return obj.map(e => evaluate.fromJSON(e, seenEls, window));
 
       // web elements
       } else if (Object.keys(obj).includes(element.Key) ||
           Object.keys(obj).includes(element.LegacyKey)) {
         /* eslint-disable */
         let uuid = obj[element.Key] || obj[element.LegacyKey];
-        let el = seenEls.get(uuid);
+        let el = seenEls.get(uuid, window);
         /* eslint-enable */
         if (!el) {
           throw new WebDriverError(`Unknown element: ${uuid}`);
         }
         return el;
 
       }
 
       // arbitrary objects
       let rv = {};
       for (let prop in obj) {
-        rv[prop] = evaluate.fromJSON(obj[prop], seenEls, win, shadowRoot);
+        rv[prop] = evaluate.fromJSON(obj[prop], seenEls, window);
       }
       return rv;
   }
 };
 
 /**
  * Convert arbitrary objects to JSON-safe primitives that can be
  * transported over the Marionette protocol.
--- a/testing/marionette/listener.js
+++ b/testing/marionette/listener.js
@@ -844,17 +844,17 @@ function emitTouchEvent(type, touch) {
         0);
   }
 }
 
 /**
  * Function that perform a single tap
  */
 async function singleTap(id, corx, cory) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   // after this block, the element will be scrolled into view
   let visible = element.isVisible(el, corx, cory);
   if (!visible) {
     throw new ElementNotInteractableError(
         "Element is not currently visible and may not be manipulated");
   }
 
   let a11y = accessibility.get(capabilities.get("moz:accessibilityChecks"));
@@ -994,17 +994,17 @@ function setDispatch(batches, touches, b
   batchIndex++;
   for (let i = 0; i < batch.length; i++) {
     pack = batch[i];
     touchId = pack[0];
     command = pack[1];
 
     switch (command) {
       case "press":
-        el = seenEls.get(pack[2]);
+        el = seenEls.get(pack[2], curContainer.frame);
         c = element.coordinates(el, pack[3], pack[4]);
         touch = createATouch(el, c.x, c.y, touchId);
         multiLast[touchId] = touch;
         touches.push(touch);
         emitMultiEvents("touchstart", touch, touches);
         break;
 
       case "release":
@@ -1012,17 +1012,17 @@ function setDispatch(batches, touches, b
         // the index of the previous touch for the finger may change in
         // the touches array
         touchIndex = touches.indexOf(touch);
         touches.splice(touchIndex, 1);
         emitMultiEvents("touchend", touch, touches);
         break;
 
       case "move":
-        el = seenEls.get(pack[2]);
+        el = seenEls.get(pack[2], curContainer.frame);
         c = element.coordinates(el);
         touch = createATouch(multiLast[touchId].target, c.x, c.y, touchId);
         touchIndex = touches.indexOf(lastTouch);
         touches[touchIndex] = touch;
         multiLast[touchId] = touch;
         emitMultiEvents("touchmove", touch, touches);
         break;
 
@@ -1267,17 +1267,17 @@ function getPageSource() {
  */
 async function findElementContent(strategy, selector, opts = {}) {
   if (!SUPPORTED_STRATEGIES.has(strategy)) {
     throw new InvalidSelectorError("Strategy not supported: " + strategy);
   }
 
   opts.all = false;
   if (opts.startNode) {
-    opts.startNode = seenEls.get(opts.startNode);
+    opts.startNode = seenEls.get(opts.startNode, curContainer.frame);
   }
 
   let el = await element.find(curContainer, strategy, selector, opts);
   let elRef = seenEls.add(el);
   let webEl = element.makeWebElement(elRef);
   return webEl;
 }
 
@@ -1287,17 +1287,17 @@ async function findElementContent(strate
  */
 async function findElementsContent(strategy, selector, opts = {}) {
   if (!SUPPORTED_STRATEGIES.has(strategy)) {
     throw new InvalidSelectorError("Strategy not supported: " + strategy);
   }
 
   opts.all = true;
   if (opts.startNode) {
-    opts.startNode = seenEls.get(opts.startNode);
+    opts.startNode = seenEls.get(opts.startNode, curContainer.frame);
   }
 
   let els = await element.find(curContainer, strategy, selector, opts);
   let elRefs = seenEls.addAll(els);
   let webEls = elRefs.map(element.makeWebElement);
   return webEls;
 }
 
@@ -1328,79 +1328,79 @@ function clickElement(msg) {
     let target = getElementAttribute(id, "target");
 
     if (target === "_blank") {
       loadEventExpected = false;
     }
 
     loadListener.navigate(() => {
       return interaction.clickElement(
-          seenEls.get(id),
+          seenEls.get(id, curContainer.frame),
           capabilities.get("moz:accessibilityChecks"),
           capabilities.get("moz:webdriverClick")
       );
     }, commandID, pageTimeout, loadEventExpected, true);
 
   } catch (e) {
     sendError(e, commandID);
   }
 }
 
 function getElementAttribute(id, name) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   if (element.isBooleanAttribute(el, name)) {
     if (el.hasAttribute(name)) {
       return "true";
     }
     return null;
   }
   return el.getAttribute(name);
 }
 
 function getElementProperty(id, name) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   return typeof el[name] != "undefined" ? el[name] : null;
 }
 
 /**
  * Get the text of this element. This includes text from child elements.
  *
  * @param {WebElement} id
  *     Reference to web element.
  *
  * @return {string}
  *     Text of element.
  */
 function getElementText(id) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   return atom.getElementText(el, curContainer.frame);
 }
 
 /**
  * Get the tag name of an element.
  *
  * @param {WebElement} id
  *     Reference to web element.
  *
  * @return {string}
  *     Tag name of element.
  */
 function getElementTagName(id) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   return el.tagName.toLowerCase();
 }
 
 /**
  * Determine the element displayedness of the given web element.
  *
  * Also performs additional accessibility checks if enabled by session
  * capability.
  */
 function isElementDisplayed(id) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   return interaction.isElementDisplayed(
       el, capabilities.get("moz:accessibilityChecks"));
 }
 
 /**
  * Retrieves the computed value of the given CSS property of the given
  * web element.
  *
@@ -1408,32 +1408,32 @@ function isElementDisplayed(id) {
  *     Web element reference.
  * @param {String} prop
  *     The CSS property to get.
  *
  * @return {String}
  *     Effective value of the requested CSS property.
  */
 function getElementValueOfCssProperty(id, prop) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   let st = curContainer.frame.document.defaultView.getComputedStyle(el);
   return st.getPropertyValue(prop);
 }
 
 /**
  * Get the position and dimensions of the element.
  *
  * @param {WebElement} id
  *     Reference to web element.
  *
  * @return {Object.<string, number>}
  *     The x, y, width, and height properties of the element.
  */
 function getElementRect(id) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   let clientRect = el.getBoundingClientRect();
   return {
     x: clientRect.x + curContainer.frame.pageXOffset,
     y: clientRect.y + curContainer.frame.pageYOffset,
     width: clientRect.width,
     height: clientRect.height,
   };
 }
@@ -1443,50 +1443,50 @@ function getElementRect(id) {
  *
  * @param {WebElement} id
  *     Reference to web element.
  *
  * @return {boolean}
  *     True if enabled, false otherwise.
  */
 function isElementEnabled(id) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   return interaction.isElementEnabled(
       el, capabilities.get("moz:accessibilityChecks"));
 }
 
 /**
  * Determines if the referenced element is selected or not.
  *
  * This operation only makes sense on input elements of the Checkbox-
  * and Radio Button states, or option elements.
  */
 function isElementSelected(id) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   return interaction.isElementSelected(
       el, capabilities.get("moz:accessibilityChecks"));
 }
 
 async function sendKeysToElement(id, val) {
-  let el = seenEls.get(id);
+  let el = seenEls.get(id, curContainer.frame);
   if (el.type == "file") {
     await interaction.uploadFile(el, val);
   } else if ((el.type == "date" || el.type == "time") &&
       Preferences.get("dom.forms.datetime")) {
     interaction.setFormControlValue(el, val);
   } else {
     await interaction.sendKeysToElement(
         el, val, false, capabilities.get("moz:accessibilityChecks"));
   }
 }
 
 /** Clear the text of an element. */
 function clearElement(id) {
   try {
-    let el = seenEls.get(id);
+    let el = seenEls.get(id, curContainer.frame);
     if (el.type == "file") {
       el.value = null;
     } else {
       atom.clearElement(el, curContainer.frame);
     }
   } catch (e) {
     // Bug 964738: Newer atoms contain status codes which makes wrapping
     // this in an error prototype that has a status property unnecessary
@@ -1522,17 +1522,17 @@ function switchToShadowRoot(id) {
         parent = parent.parentNode;
       }
       curContainer.shadowRoot = parent;
     }
     return;
   }
 
   let foundShadowRoot;
-  let hostEl = seenEls.get(id);
+  let hostEl = seenEls.get(id, curContainer.frame);
   foundShadowRoot = hostEl.shadowRoot;
   if (!foundShadowRoot) {
     throw new NoSuchElementError("Unable to locate shadow root: " + id);
   }
   curContainer.shadowRoot = foundShadowRoot;
 }
 
 /**
@@ -1589,17 +1589,17 @@ function switchToFrame(msg) {
     sendOk(commandID);
     return;
   }
 
   let id = msg.json.element;
   if (seenEls.has(id)) {
     let wantedFrame;
     try {
-      wantedFrame = seenEls.get(id);
+      wantedFrame = seenEls.get(id, curContainer.frame);
     } catch (e) {
       sendError(e, commandID);
     }
 
     if (frames.length > 0) {
       for (let i = 0; i < frames.length; i++) {
         // use XPCNativeWrapper to compare elements; see bug 834266
         let frameEl = frames[i].frameElement;
@@ -1722,34 +1722,35 @@ function switchToFrame(msg) {
  *     Base64 encoded string or a SHA-256 hash of the screenshot.
  */
 function takeScreenshot(format, opts = {}) {
   let id = opts.id;
   let full = !!opts.full;
   let highlights = opts.highlights || [];
   let scroll = !!opts.scroll;
 
-  let highlightEls = highlights.map(ref => seenEls.get(ref));
+  let win = curContainer.frame;
+  let highlightEls = highlights.map(ref => seenEls.get(ref, win));
 
   let canvas;
 
   // viewport
   if (!id && !full) {
-    canvas = capture.viewport(curContainer.frame, highlightEls);
+    canvas = capture.viewport(win, highlightEls);
 
   // element or full document element
   } else {
     let el;
     if (id) {
-      el = seenEls.get(id);
+      el = seenEls.get(id, win);
       if (scroll) {
         element.scrollIntoView(el);
       }
     } else {
-      el = curContainer.frame.document.documentElement;
+      el = win.document.documentElement;
     }
 
     canvas = capture.element(el, highlightEls);
   }
 
   switch (format) {
     case capture.Format.Base64:
       return capture.toBase64(canvas);