Stale elements do not work since CPG (compartment per global) has been landed

RESOLVED FIXED

Status

defect
P2
normal
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=2][mozmill-2.0+])

Attachments

(1 attachment)

On bug 610258 I have added a feature to better handle stale elements for content pages. So we simply cached the window in the element class. With the GPC landing that's not possible anymore because we would work on a dead object after a reload of the tab.

Probably we should get rid of this handling at all and backout the patch on bug 610258 for master. Tests really should re-instantiate elements after a page load.

Clint, what do you think?
Summary: Stale elements do not work since GPC has been landed → Stale elements do not work since CPG has been landed
I'm with you, I think we should remove the ability to re-use stale elements and force the test to recreate them. But it would be nice if the framework could let you know when an element goes stale.
Whiteboard: [mozmill-2.0?] → [mozmill-2.0-][mozmill-next]
Lets bring this back to the triage meeting today, given that it is a regression from Mozmill 1.5.
Whiteboard: [mozmill-2.0-][mozmill-next] → [mozmill-2.0?]
Priority: -- → P2
Summary: Stale elements do not work since CPG has been landed → Stale elements do not work since CPG (compartment per global) has been landed
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Whiteboard: [mozmill-2.0+] → [ateamtrack: p=mozmill q=2013q2 m=2][mozmill-2.0+]
We mixed up the bug assignments on Monday. That one can actually be done by Johannes while I have to work on bug 760720.
Assignee: hskupin → mozilla_dev
Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=2][mozmill-2.0+] → [ateamtrack: p=mozmill q=2013q2 m=2][mozmill-2.0+][mentor:whimboo][lang:js]
Blocks: 860655
Ok, with :whimboo 's help I have done several changes similar to the ones for bug 610258 .

Unfortunately, this has not completely solved the issue yet and I have a strange situation, now.

Here is the link to the current commits on my github fork

https://github.com/Nebelhom/mozmill/commits/bug-761600-fix

When running stale_elements.js as is on an aurora build with

mozmill -b ./firefox-aurora/firefox/firefox-bin -t mutt/mutt/tests/js/elementslib/stale_element.js

it passes (Please note the change of assert.ok to simply elem.getNode() )

However, when uncommenting the following line:

https://github.com/Nebelhom/mozmill/blob/bug-761600-fix/mozmill/mozmill/extension/resource/driver/mozelement.js#l87

the test times out. Henrik suggested to not cache the document, but the defaultView in this._view to fix the issue, but instead the addition seems to have a rather drastic. I don't understand why the mere addition of a variable definition would have such a drastic effect...

Could anyone of you tell me the reason for this behaviour and/or how to fix it? Thanks

Johannes
Ok, an error that seems to occur during the above mentioned procedure is:

Timestamp: 04/24/2013 11:50:25 PM
Error: [Exception... "'TypeError: args.document is undefined' when calling method: [nsITimerCallback::notify]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

Therefore args["document"].defaultView would understandably not work.

Not sure, how to circumvent this problem just now. Suggestions are welcome.
Assignee: mozilla_dev → nobody
Status: ASSIGNED → NEW
Assignee: nobody → hskupin
Posted patch Patch v1Splinter Review
With the class hierarchy fixed in bug 860658 this was an easy modification. We simply also cache the window and each time we want to access the element, we check if the document is valid. If not we retrieve the element and cache it again.

Keep in mind that this will not work for Elem nodes because we do not have any way to retrieve it again after the document has been replaced. In this case we return null for getNode() and don't throw the dead object error. Which is already very helpful.
Attachment #744920 - Flags: review?(ctalbert)
Comment on attachment 744920 [details] [diff] [review]
Patch v1

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

I think these changes look fine.
Attachment #744920 - Flags: review?(ctalbert) → review+
Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=2][mozmill-2.0+][mentor:whimboo][lang:js] → [ateamtrack: p=mozmill q=2013q2 m=2][mozmill-2.0+]
Pushed to master:
https://github.com/mozilla/mozmill/commit/1706822f1d7b336a98b6622e8148a761e840f1a8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.