waitForPageLoad() fails if the document owner is not a tab but an HTMLDocument (add support for iframes)

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: vladmaniac, Assigned: whimboo)

Tracking

({regression})

unspecified
regression
Dependency tree / graph
Bug Flags:
in-testsuite ?

Details

(Whiteboard: [mozmill-1.5.4+][mozmill-2.0+])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 534445 [details]
script example 1.0

Build ID: 
Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

controller.waitForPageLoad(controller.tabs.activeTab.defaultView.frames[0].document) causes test to fail. The workaround would be to use controller.sleep(time_value), but we all know this is bad practice. 

See attachment for script example.
(Assignee)

Comment 1

7 years ago
As suspected earlier that's a regression from bug 604878 or a related one. At this time we caused a regression when explicitly checking for a tab. It will fail for iframes. I will probably have to take a look at it today.

Vlad, for now use waitForElement to wait for a specific element.
Assignee: nobody → hskupin
Blocks: 604878
Status: NEW → ASSIGNED
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
Summary: waitForPageLoad() does not handle iframes → waitForPageLoad() fails for iFrames with 'Specified tab hasn't been found.'
(Assignee)

Comment 2

7 years ago
This needs to be fixed in Mozmill 1.5.4.
Whiteboard: [mozmill-1.5.4?]
(Assignee)

Comment 3

7 years ago
So what's the current state what we have right now: On the window object and for tabs we have attached event listeners for the loaded state. Those handle the .documentLoaded property. It works pretty well for both cases.

Something we missed so far are iframes. It's easy to enable the right handling in waitForPageLoad() but it will not work until we correctly set the documentLoaded property. Right now I'm not sure what's the easiest way to set this property.

A first thought started me to think about the following way: We should work with the tab onload listener, iterate over possible iframes in the tab window, and attach further listeners onto each iframe. That way we can track the loading state of each single iframe inside a document.

Frankly I'm not quite sure if this is the best approach for it or if we have a much cleaner way. Clint, what do you think?
(Assignee)

Comment 4

7 years ago
window.gBrowser.addEventListener already handles iframes. So I would have to find a way to correctly distinguish between tabs and iframes.
(Assignee)

Comment 5

7 years ago
Ok, after checking with DOMi (which I should have done already earlier) there are no iframes involved here. It's a simple embedded browser. If Clint is not able to answer until tomorrow morning, I will have a further look.
Summary: waitForPageLoad() fails for iFrames with 'Specified tab hasn't been found.' → waitForPageLoad() fails for Discovery Pane with 'Specified tab hasn't been found.'
(Assignee)

Comment 6

7 years ago
Ok, I have it working but there is still an issue how pages are getting loaded:

-> Close tab:
*** Unload tab: location=about:blank, baseURI=about:blank
*** Unload HTML location=about:blank, baseURI=about:addons
-> Open Addons Manager
*** Loaded HTML location=about:blank, baseURI=about:addons
*** Loaded tab: location=about:addons, baseURI=about:addons
*** Unload HTML location=about:blank, baseURI=about:addons
*** Loaded HTML location=https://services.addons.mozilla.org/en-US/...

As you can see in the second part we load 'about:blank' and see an onLoad event for the HTML page. Right after the tab notices that it has been finished loading and we unload the temporary 'about:blank' page. Right after we load the Discovery Pane.

Not sure yet, what's the best way to get this solved. Will further investigate.
(Assignee)

Comment 7

7 years ago
Ok, so the basic fix for Mozmill has to cover the correct distinction between tab documents and HTML documents. The specific issues for the discover pane have to be addressed in a separate bug and need a fix in our own shared modules.

Patch with new and updated tests upcoming.
Flags: in-testsuite?
Summary: waitForPageLoad() fails for Discovery Pane with 'Specified tab hasn't been found.' → waitForPageLoad() fails if the document owner is not a tab but an HTMLDocument
(Assignee)

Comment 8

7 years ago
Created attachment 534714 [details] [diff] [review]
Patch v1
Attachment #534445 - Attachment is obsolete: true
Attachment #534714 - Flags: review?(ctalbert)
(Assignee)

Comment 9

7 years ago
Created attachment 534720 [details] [diff] [review]
Patch v1.1

With my latest qrefresh applied. I left in the debug statements in init.js but simply comment those out. I had to use those a lot in the past, so they will probably be helpful again at some point.

Otherwise this patch doesn't regress any of our existing tests and also the attached tests pass.
Attachment #534714 - Attachment is obsolete: true
Attachment #534714 - Flags: review?(ctalbert)
Attachment #534720 - Flags: review?(ctalbert)
(Assignee)

Comment 10

7 years ago
Oh, it can be that this patch will even work for frames. But I don't have time at the moment to test this in detail.

Updated

7 years ago
Whiteboard: [mozmill-1.5.4?] → [mozmill-1.5.4+]
(Assignee)

Comment 11

7 years ago
Clint, would be great if we can get this reviewed before Heather starts to work on the fixes for the last review on AMO.
Whiteboard: [mozmill-1.5.4+] → [mozmill-1.5.4+][mozmill-2.0+]

Comment 12

7 years ago
Comment on attachment 534720 [details] [diff] [review]
Patch v1.1

This patch looks good, Henrik.  I modified it to add in a test for a site using iframes and it worked.  So I've changed the test and removed the "TODO: check for iframes" comment in controller.js before checking in.
Attachment #534720 - Flags: review?(ctalbert) → review+
(Assignee)

Updated

7 years ago
Summary: waitForPageLoad() fails if the document owner is not a tab but an HTMLDocument → waitForPageLoad() fails if the document owner is not a tab but an HTMLDocument (add support for iframes)

Comment 13

7 years ago
Created attachment 535258 [details] [diff] [review]
Port to master

Here is a first draft port to master.

I was interested in doing this because of your assertions on IRC that it isn't working for iframes (and also because I realised the test needs some porting to the new controller APIs).  

I'm seeing this output:
INFO | Step Pass: {
  "function": "Controller.open()"
}
***Loaded HTML: location=http://people.mozilla.org/~ctalbert/testpages/iframesrc.html, baseURI=http://people.mozilla.org/~ctalbert/testpages/iframesrc.html
***Loaded tab: location=http://people.mozilla.org/~ctalbert/testpages/iframetest.html, baseURI=http://people.mozilla.org/~ctalbert/testpages/iframetest.html

That is telling me that it looks like we are seeing the onload event from inside the iframe (iframesrc.html is the file loaded into the iframe).  

I need to complete the test to do an assertNode on the two IDs - one on the main iframe page, and one inside the iframe to see how far we can go.  There are other test changes too - I had to comment out two element lookups because I can't get Elementslib to find those elements regardless of what I do.  I'll have to look at that a little more because it could either be (A) a bug in the new elementslib or (B) a bug in the way that I've implemented your changes inside init.js.  

The two element lookups that are broken are: test_waitForPageLoad.js line 107-108 and line 131 in the patched file.  (Apply the patch then look for those lines).

But the good news is that with your changes the addons discovery pane does seem to work.  (Tested on Firefox 4 build, linux).
Attachment #535258 - Flags: feedback?(hskupin)

Comment 14

7 years ago
Also, because it was urgent, landed the 1.5.x patch onto the hotfix-1.5 branch: https://github.com/mozautomation/mozmill/commit/c0d511bca4362cef9c6346abf5b3e697839c3f5f
(Assignee)

Comment 15

7 years ago
Comment on attachment 535258 [details] [diff] [review]
Port to master

>- //{url : "https://mozilla.org/", type: "id", value : "getMeOutOfHereButton"}
>+ {url : "https://mozilla.org/", type: "id", value : "getMeOutOfHereButton"},
>+
>+ // Iframe pages
>+ {url : "http://people.mozilla.org/~ctalbert/testpages/iframetest.html", type: "id", value: "iframe"},
>+ //{url : "http://people.mozilla.org/~ctalbert/testpages/iframetest.html", type: "id", value: "innerid"}

I still think we should/can not handle this case in the general waitForPageLoad case and therefore can't have it inside this array. It should become part of PART VIII:

* Update your testcase so it uses a remote page which takes longer to load
* Open the example page and wait for the page being loaded via activeTab as document
* Find the document of the embedded iframe
* Wait for page being loaded for the iframe document
* Test an element inside the iframe

Only with those steps we can fully test this feature.

>-  controller.keypress(null, "t", {accelKey: true});
>+
>+  var elem_win = new elementslib.MozMillElement("Elem", controller.window); 
>+  controller.keypress(elem_win, "t", {accelKey: true});

Just keep it as it was before. 'null' will send it to the window automatically. Or does that not work anymore with 2.0?

> ***Loaded HTML:
> location=http://people.mozilla.org/~ctalbert/testpages/iframesrc.html,
> baseURI=http://people.mozilla.org/~ctalbert/testpages/iframesrc.html
> ***Loaded tab:
> location=http://people.mozilla.org/~ctalbert/testpages/iframetest.html,
> baseURI=http://people.mozilla.org/~ctalbert/testpages/iframetest.html

That looks great, so we really only have to do the above test to make sure waitForPageLoad will succeed for the iframes document.

> The two element lookups that are broken are: test_waitForPageLoad.js line
> 107-108 and line 131 in the patched file.  (Apply the patch then look for
> those lines).

Instead of 'area' you can use 'id: get-involved-interest'. But it sounds like a bug in how the new elementslib retrieves elements as Name.

And for 'controller.assertNode(element)' I strongly believe it's another regression in Mozmill 2.
Attachment #535258 - Flags: feedback?(hskupin) → feedback-

Updated

7 years ago
Blocks: 661408
(Assignee)

Comment 16

7 years ago
Comment on attachment 535258 [details] [diff] [review]
Port to master

Obsoleting attachment. We have to land the final test on the hotfix-1.5 branch first and then an update of the patch is necessary.
Attachment #535258 - Attachment is obsolete: true
(Assignee)

Comment 17

7 years ago
Created attachment 545771 [details] [diff] [review]
Patch v1 (iframe test)

This patch includes the correct iFrame test. Also we can now remove the flaky discovery pane test which is basically the same as an iframe test.
Attachment #545771 - Flags: review?(fayearthur+bugs)
(Assignee)

Comment 18

7 years ago
Created attachment 545775 [details] [diff] [review]
Patch v1.1 (iframe test)

Now with the iframe.html file included.
Attachment #545771 - Attachment is obsolete: true
Attachment #545771 - Flags: review?(fayearthur+bugs)
Attachment #545775 - Flags: review?(fayearthur+bugs)
Comment on attachment 545775 [details] [diff] [review]
Patch v1.1 (iframe test)

tests are good.
Attachment #545775 - Flags: review?(fayearthur+bugs) → review+
(Assignee)

Comment 20

7 years ago
The iframe test has been landed as:
https://github.com/mozautomation/mozmill/commit/61a725be625a37abde63b23a366ea01d7f15217a

I will come up with a mozmill-2.0 patch by tomorrow.
(Assignee)

Comment 21

7 years ago
Created attachment 546171 [details] [diff] [review]
Patch v1 (combined, mozmill-2.0)

This patch combines all code changes from Mozmill 1.5.x.

I have one problem here with the part VI. When I want to retrieve an element from a background tab, I don't get it to work. That's not because of waitForPageLoad changes, it also fails with a longer sleep. Andrew, could that be somehow related to the MozMillElement changes? I have no idea, what's the reason for is. Would be great if you could help out.
Attachment #546171 - Flags: review?(fayearthur+bugs)
Attachment #546171 - Flags: feedback?(halbersa)
Comment on attachment 546171 [details] [diff] [review]
Patch v1 (combined, mozmill-2.0)

r+ with these things:

>+    var owner = null;

you can just do `var owner;` here fwiw

>+    if (tab) {
>+      //dump("*** Loaded tab: location=" + doc.location + ", baseURI=" + doc.baseURI + "\n");
>       tab.mozmillDocumentLoaded = true;
>+    } else {
>+      //dump("*** Loaded HTML location=" + doc.location + ", baseURI=" + doc.baseURI + "\n");
>+      doc.defaultView.mozmillDocumentLoaded = true;
>+    }
>

Remove all these commented out dump statements

>+  // Once the iframe has been loaded assert that the element exists
>+  var home = new elementslib.ID(frameController.window.document, "home-fx");
>+  frameController.assertNode(home);
> }
> 

assertNode() is being deprecated, so I don't think we should be adding any tests with it (if this is for 2.0)
Attachment #546171 - Flags: review?(fayearthur+bugs) → review+
(Assignee)

Comment 23

7 years ago
(In reply to comment #22)
> >+    if (tab) {
> >+      //dump("*** Loaded tab: location=" + doc.location + ", baseURI=" + doc.baseURI + "\n");
> >       tab.mozmillDocumentLoaded = true;
> >+    } else {
> >+      //dump("*** Loaded HTML location=" + doc.location + ", baseURI=" + doc.baseURI + "\n");
> >+      doc.defaultView.mozmillDocumentLoaded = true;
> >+    }
> 
> Remove all these commented out dump statements

As spoken with Clint we wanted to leave this code in that file but comment it out. Debugging the loaded state is awkward and having only to uncomment the code is a big help.

Comment 24

7 years ago
(In reply to comment #23)
> (In reply to comment #22)
> > >+    if (tab) {
> > >+      //dump("*** Loaded tab: location=" + doc.location + ", baseURI=" + doc.baseURI + "\n");
> > >       tab.mozmillDocumentLoaded = true;
> > >+    } else {
> > >+      //dump("*** Loaded HTML location=" + doc.location + ", baseURI=" + doc.baseURI + "\n");
> > >+      doc.defaultView.mozmillDocumentLoaded = true;
> > >+    }
> > 
> > Remove all these commented out dump statements
> 
> As spoken with Clint we wanted to leave this code in that file but comment
> it out. Debugging the loaded state is awkward and having only to uncomment
> the code is a big help.

Yep, that was what I said originally.  Those are useful when triaging through this code because you can easily get a trace of how we attach to every window and tab.  We've been taking them out every time we land a change to this code and then putting them right back in afterward when we need to debug it.  We've done that three or four times now.  

Normally I'm with you on this Harth, but I'd say let's leave them in in this one case.
(Assignee)

Comment 25

7 years ago
Created attachment 547173 [details] [diff] [review]
Patch v2 (mozmill-2.0)

Updated patch with review comments. I got rid of all old element getters and assertions. Also I have switched from the dump statements to log, which should be used for Mozmill 2.

I still have problems with checking content in the background tab. I will file that as a separate bug. Also httpd is broken on my machine. I can't create any connection anymore. Same here, I will file a new bug.
Attachment #546171 - Attachment is obsolete: true
Attachment #546171 - Flags: feedback?(halbersa)
Attachment #547173 - Flags: review?(fayearthur+bugs)
Comment on attachment 547173 [details] [diff] [review]
Patch v2 (mozmill-2.0)

Cool.
Attachment #547173 - Flags: review?(fayearthur+bugs) → review+
(Assignee)

Comment 27

7 years ago
Finally fixed on master:
https://github.com/mozautomation/mozmill/commit/aa39e06053d8c1f5e6548d144e0c9871c53951d7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.