760720 - waitForPageLoad() method still augments elements into window object

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As you can see in the following link (or bug 686030) we are still setting a custom property on the window object. Given the GPC feature this can't be done anymore. I will have to check how we can handle that correctly. 

https://github.com/mozautomation/mozmill/commit/0b4c4deb646df5615e86dac1d1019329d6c757d2#L0L954

Probably I will include the fix in my patch for bug 760418 where I completely reorganize the tests.

Comment 1

[cmtalbert]

Nebelhom - let us know if you have any questions

Comment 2

[Johannes Vogel (:nebelhom)]

@ctalbert , @whimboo

The link gives me a 404

https://github.com/mozautomation/mozmill/commit/0b4c4deb646df5615e86dac1d1019329d6c757d2#L0L954

Has the repository moved?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes, it's now available under the mozilla account.
https://github.com/mozilla/mozmill/commit/0b4c4deb646df5615e86dac1d1019329d6c757d2#L0L954

Comment 4

[Johannes Vogel (:nebelhom)]

I'm afraid, I would have to ask for some clarification :(

I've had a read through all the links etc. but some things are still a little unclear so far:

1) https://bugzilla.mozilla.org/attachment.cgi?id=559592&action=edit

Is this the test case that I can use to test if my fix works (It is an attachment of bug 686030 which fails for me when used as a test case with the latest version)

2) I wanted to read up on the GPC feature that is mentioned, but I can't find it. Could someone maybe supply a link for light background reading?

Thanks a lot for the help.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Johannes Vogel (:nebelhom) from comment #4)
> I'm afraid, I would have to ask for some clarification :(
> 
> I've had a read through all the links etc. but some things are still a
> little unclear so far:
> 
> 1) https://bugzilla.mozilla.org/attachment.cgi?id=559592&action=edit
> 
> Is this the test case that I can use to test if my fix works (It is an
> attachment of bug 686030 which fails for me when used as a test case with
> the latest version)

As I understand it, we're adding a property to the window object, but due to CPG once we refresh we will lose this property. We would probably expect a failure due to "controller.waitForPageLoad(): Timeout waiting for page loaded."

See: https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L1036

We added a windowMap for storing when a window is loaded, but we appear to be setting mozmillWaitForPageLoad. This was added as per bug 686030 comment 9. It may be worth asking Andrew's thoughts on this.

> 2) I wanted to read up on the GPC feature that is mentioned, but I can't
> find it. Could someone maybe supply a link for light background reading?

It's CPG, and here's a blog post about it: https://bholley.wordpress.com/2012/05/04/at-long-last-compartment-per-global/

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We mixed up the bug assignments on Monday. That one should actually be my bug while Johannes can work on bug 761600.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1

With this patch we are changing the waitForPageLoad method fundamentally. The method we base on right now is totally unreliable and causes problems in various areas. With my changes we now introduce an uuid for each page load. With it we can identify if we already called the method for the current page and don't return immediately. Whenever waitForPageLoad gets called now, we will not fail anymore. All the broken tests are passing now.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1.1

By checking the patch I noticed that I missed a call to update_loaded_status() in onWindowLoaded.

Comment 9

[cmtalbert]

Comment on attachment 744710 [details] [diff] [review]
Patch v1.1

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

There is something wrong with this patch. I think we might be missing a file or two.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1.2

Damn. The proposed way to export a patch for bugzilla is 'git show' but that only exports the latest commit. :/ Now the new patch rebased first.

Comment 11

[cmtalbert]

Comment on attachment 744741 [details] [diff] [review]
Patch v1.2

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

So, what you've really got here is a small state machine, but you've flattened it out so that it doesn't appear like one.  I think the code would be far more understandable if you actually treated it like a real state machine and made your transitions more clear.

As written the code is really hard to follow and I can't let that pass. I'd love to see all the changes to the windowmap consolidated into mozmill.js. I'd love to see the changes of windowmap actually following a defined state machine in order to make it clear what is happening so that other people can hack on this as well later.

I think the idea behind it is solid, but the code is just not quite clear enough yet. So, r-.

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +419,5 @@
>    var win = aWindow || this.window;
>  
>    var id = utils.getWindowId(win);
> +  var load_id = windowMap.getValue(id, "load_uuid");
> +  var loaded_id = windowMap.getValue(id, "loaded_uuid");

These variable names make this really, really hard to follow. It seems that "load_uuid" is your "I've been loaded but I haven't finished yet OR I've been unloaded because I am being refreshed OR I've been unloaded for some other reason" indicator. And "loaded_uuid" is the "I'm done being loaded now" indicator.  If that's correct can we make the change to call the load_uuid something like "load_in_transition" (or something like that, I kind of hate that name too) and call the "loaded_uuid" something like "load_completed"

@@ +423,5 @@
> +  var loaded_id = windowMap.getValue(id, "loaded_uuid");
> +
> +  var isLoaded = windowMap.contains(id) && windowMap.getValue(id, "loaded") &&
> +                 ((load_id === undefined) || (load_id !== loaded_id));
> +

This really needs a comment. It took me quite a bit to parse what was going on.

@@ +426,5 @@
> +                 ((load_id === undefined) || (load_id !== loaded_id));
> +
> +  if (isLoaded) {
> +    // Backup the current uuid so we can check later if another page load happened.
> +    windowMap.update(id, "loaded_uuid", load_id);

I really don't like that controller.isloaded() is modifying the underlying object store. That really feels like a side-effect. I'd much prefer that controller call back into mozmill.js and have mozmill.js handle this change just so that way we know exactly where and when the windowmap object is changing. Ideally we would use the same update_window_status function to do this.

::: mozmill/mozmill/extension/resource/driver/mozmill.js
@@ +327,5 @@
> +    controller.windowMap.update(id, "loaded", aLoaded);
> +
> +    var uuid = controller.windowMap.getValue(id, "load_uuid");
> +
> +    // When the page gets unloaded create a new uuid

Is this so that when we refresh we get a new uuid in the load_uuid attribute so that we can determine when the new page finishes loading?  In other words, we depend on the "onunload" handler firing during the refresh cycle. That dependency should be called out here, if so.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Clint Talbert ( :ctalbert ) from comment #11)
> So, what you've really got here is a small state machine, but you've
> flattened it out so that it doesn't appear like one.  I think the code would
> be far more understandable if you actually treated it like a real state
> machine and made your transitions more clear.
> 
> As written the code is really hard to follow and I can't let that pass. I'd
> love to see all the changes to the windowmap consolidated into mozmill.js.
> I'd love to see the changes of windowmap actually following a defined state
> machine in order to make it clear what is happening so that other people can
> hack on this as well later.
> 
> I think the idea behind it is solid, but the code is just not quite clear
> enough yet. So, r-.

Fair enough. I thought something similar but decided against it for the patch, given that the windowMap has only basic methods to handle entries. We can make it a rich object and add even more specialized methods, if that is what you mean here. In that case I would move the whole implementation over into mozmill.js which should be the better place. With that change we can certainly improve the code and hide the implementation details for consumers. Please let me know what you think about.

> ::: mozmill/mozmill/extension/resource/driver/controller.js
> @@ +419,5 @@
> >    var win = aWindow || this.window;
> >  
> >    var id = utils.getWindowId(win);
> > +  var load_id = windowMap.getValue(id, "load_uuid");
> > +  var loaded_id = windowMap.getValue(id, "loaded_uuid");
> 
> These variable names make this really, really hard to follow. It seems that
> "load_uuid" is your "I've been loaded but I haven't finished yet OR I've
> been unloaded because I am being refreshed OR I've been unloaded for some
> other reason" indicator. And "loaded_uuid" is the "I'm done being loaded
> now" indicator.  If that's correct can we make the change to call the
> load_uuid something like "load_in_transition" (or something like that, I
> kind of hate that name too) and call the "loaded_uuid" something like
> "load_completed"

It's not about a load still happening and a load done, but if the waitForPageLoad method  has checked the latest page load. But before talking about the naming convention we should decide on where all this code has to live (see above).

> @@ +423,5 @@
> > +  var loaded_id = windowMap.getValue(id, "loaded_uuid");
> > +
> > +  var isLoaded = windowMap.contains(id) && windowMap.getValue(id, "loaded") &&
> > +                 ((load_id === undefined) || (load_id !== loaded_id));
> > +
> 
> This really needs a comment. It took me quite a bit to parse what was going
> on.

If we consolidate everything in the windowMap itself, we could hide those details. But yes, it should be made clearer.

> @@ +426,5 @@
> > +                 ((load_id === undefined) || (load_id !== loaded_id));
> > +
> > +  if (isLoaded) {
> > +    // Backup the current uuid so we can check later if another page load happened.
> > +    windowMap.update(id, "loaded_uuid", load_id);
> 
> I really don't like that controller.isloaded() is modifying the underlying
> object store. That really feels like a side-effect. I'd much prefer that
> controller call back into mozmill.js and have mozmill.js handle this change
> just so that way we know exactly where and when the windowmap object is
> changing. Ideally we would use the same update_window_status function to do
> this.

We cannot re-use update_window_status here given that is the other side of the synchronization code. isLoaded() or waitForPageLoad() has to get its own method. But that opens a good question, which of those two messages should actually update the map? For content waitForPageLoad would be fine but given that we would also need this for chrome, and we call it in the MozMillController constructor, I wonder what's the best place. I would keep it in isLoaded().

> ::: mozmill/mozmill/extension/resource/driver/mozmill.js
> @@ +327,5 @@
> > +    controller.windowMap.update(id, "loaded", aLoaded);
> > +
> > +    var uuid = controller.windowMap.getValue(id, "load_uuid");
> > +
> > +    // When the page gets unloaded create a new uuid
> 
> Is this so that when we refresh we get a new uuid in the load_uuid attribute
> so that we can determine when the new page finishes loading?  In other
> words, we depend on the "onunload" handler firing during the refresh cycle.
> That dependency should be called out here, if so.

We depend on beforeunload and pagehide (bfcache). In both cases we set a new uuid. Lets give some examples.

1. A page has been loaded and we work with a link element
2. Click the link element which loads another web page
3. Call waitForPageLoad() right after

This will most likely fail because waitForPageLoad is called before the actual beforeunload handler is invoked, and immediately returns given that the page is still in loaded state. But with the newly generated uuid waitForPageLoad will wait until the uuid for the requested load event has been changed and the page has really been loaded.

1. A page has been loaded and we work with a link element
2. Click the link element which should load another web page
3. Call waitForPageLoad()
4. Create an element for the target website

Like above but with a broken element. A click on the link does not open the referenced URL, so we stay at the current web page. waitForPageLoad() returns immediately, and the creation of the element will fail because it doesn't exist. With the uuid we would detect that and run into a timeout for the page load.

Comment 13

[cmtalbert]

(In reply to Henrik Skupin (:whimboo) from comment #12)
> (In reply to Clint Talbert ( :ctalbert ) from comment #11)
> > So, what you've really got here is a small state machine, but you've
> > flattened it out so that it doesn't appear like one.  I think the code would
> > be far more understandable if you actually treated it like a real state
> > machine and made your transitions more clear.
> > 
> > As written the code is really hard to follow and I can't let that pass. I'd
> > love to see all the changes to the windowmap consolidated into mozmill.js.
> > I'd love to see the changes of windowmap actually following a defined state
> > machine in order to make it clear what is happening so that other people can
> > hack on this as well later.
> > 
> > I think the idea behind it is solid, but the code is just not quite clear
> > enough yet. So, r-.
> 
> Fair enough. I thought something similar but decided against it for the
> patch, given that the windowMap has only basic methods to handle entries. We
> can make it a rich object and add even more specialized methods, if that is
> what you mean here. In that case I would move the whole implementation over
> into mozmill.js which should be the better place. With that change we can
> certainly improve the code and hide the implementation details for
> consumers. Please let me know what you think about.
> 
Yes, I think this is what we want. I don't think it will be a big object, the windowmap can continue to be a simple object but one that we can use and have it handle its state transitions internally.

And yes, I think that the windowmap object ought to live in the mozmill.js file.

> We depend on beforeunload and pagehide (bfcache). In both cases we set a new
> uuid. Lets give some examples.
Thanks, those made it much more clear.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Clint Talbert ( :ctalbert ) from comment #13)
> (In reply to Henrik Skupin (:whimboo) from comment #12)
> And yes, I think that the windowmap object ought to live in the mozmill.js
> file.

It's not something which will work. By moving it into the mozmill.js file we will get a circular reference between mozmill.js and controller.js. This will break Mozmill. So we have to introduce a new module. I will call it windows.js and it should contain all the window handling code including the event handling.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v2

Updated patch with the proposed changes. As said earlier the window handling stuff has to be located in a separate module due to cyclic references. I have to agree that the current approach is way cleaner. I hope you will like it Clint.

Comment 16

[cmtalbert]

Comment on attachment 746376 [details] [diff] [review]
Patch v2

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

This looks great. I really like what you've done. r+

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +12,5 @@
> +var utils = {}; Cu.import('resource://mozmill/stdlib/utils.js', utils);
> +
> +
> +var uuidgen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> +

Nit: why the blank lines between the import statement and the global variable declaration? Don't think we need any blank lines there.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yay! Landed on master with the nits updated:
https://github.com/mozilla/mozmill/commit/af5fa510091d11101f03272ff2bb326d8cbac94a

Comment 18

[Szabolcs Hubai (:xabolcs)]

FYI without this patch most mutt js tests pass with Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130509 Firefox/23.0 ID:20130509031047 CSet: ea059733677c

RESULTS | Passed: 47
RESULTS | Failed: 3
RESULTS | Skipped: 0

But when applied, all test containing |mozmill.getBrowserController()| in setupModule got skipped:

RESULTS | Passed: 7
RESULTS | Failed: 0
RESULTS | Skipped: 44

The constructor of MozMillController is unable to detect the loaded window. [1]

> TEST-START | js/utils/pageload_bfcache.js | setupModule
> ERROR | Test Failure | {
>   "exception": {
>     "stack": [
>       "TimeoutError@resource://mozmill/stdlib/utils.js:243",
>       "waitFor@resource://mozmill/stdlib/utils.js:293",
>       "MozMillController@resource://mozmill/driver/controller.js:249",
>       "getBrowserController@resource://mozmill/driver/mozmill.js:182",
>       "setupModule@resource://mozmill/modules/frame.js -> file:///path/to/mozmill/mutt/mutt/tests/js/utils/pageload_bfcache.js:9",
>       "Runner.prototype.wrapper@resource://mozmill/modules/frame.js:633",
>       "Runner.prototype.runTestModule@resource://mozmill/modules/frame.js:678",
>       "Runner.prototype.runTestFile@resource://mozmill/modules/frame.js:603",
>       "runTestFile@resource://mozmill/modules/frame.js:750",
>       "Bridge.prototype._execFunction@resource://jsbridge/modules/Bridge.jsm:140",
>       "Bridge.prototype.execFunction@resource://jsbridge/modules/Bridge.jsm:147",
>       "@resource://jsbridge/modules/Server.jsm:33",
>       ""
>     ],
>     "message": "controller(): Window could not be initialized.",
>     "fileName": "resource://mozmill/stdlib/utils.js",
>     "name": "TimeoutError",
>     "lineNumber": 243
>   }
> }


The patch is compatible with recent Aurora 22.0a2 versions.
If this is not a top priority then I could bisect Nightly 23.0a1.


[1]: https://github.com/mozilla/mozmill/blob/fd4739f85fd7994e8e1274092d5b8d73434036c5/mozmill/mozmill/extension/resource/driver/controller.js#L241

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Szabolcs Hubai (:xabolcs) from comment #18)
> FYI without this patch most mutt js tests pass with Mozilla/5.0 (X11; Linux
> i686; rv:23.0) Gecko/20130509 Firefox/23.0 ID:20130509031047 CSet:
> ea059733677c

Please file a new bug for it and add the mozmill-2.0? flag so we can track it. Thanks.

Comment 20

[Szabolcs Hubai (:xabolcs)]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> Please file a new bug for it and add the mozmill-2.0? flag so we can track
> it. Thanks.

Filed bug 872237.