751424 - Compartment-Per-Global breaks Mozmill's mozmillDocumentLoaded property

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[John Schoenick [:johns]]

Attachment: Very simple mozmill test that tries to load a page then calls c.waitForPageLoad()

Using Mozmill 1.5.11 and the upcoming compartment-per-global patch causes tests to fail on waitForPageLoad() calls.

CPG is bug 650353 - example try push with working CPG binaries at: https://tbpl.mozilla.org/?tree=Try&rev=35de20b7eae9

It's not clear if this is CPG breakage or Mozmill breakage. Mozmill appears to stick a 'mozmillDocumentLoaded' property on document.defaultView that is then read through jsbridge, so there could be a number of interactions.

The attached mozmill test can be run via:

> mozmill -t mozmill_waitforload.js --show-errors -b some_cpg_build/firefox

Prints:

> ERROR | Test Failure: {"exception": {"stack": "TimeoutError@resource://mozmill/modules/utils.js:429\nwaitFor@resource://mozmill/modules/utils.js:467\n@resource://mozmill/modules/controller.js:648\n@resource://mozmill/modules/controller.js:1291\nsetupModule@resource://mozmill/modules/frame.js -> file:///home/johns/moz/areweslimyet/cpg_breakage.js:6\n@resource://mozmill/modules/frame.js:558\n@resource://mozmill/modules/frame.js:605\n@resource://mozmill/modules/frame.js:675\n@resource://mozmill/modules/frame.js:512\n@resource://mozmill/modules/frame.js:687\n@resource://jsbridge/modules/server.js:184\n@resource://jsbridge/modules/server.js:188\n@resource://jsbridge/modules/server.js:288\n", "message": "controller.waitForPageLoad(): Timeout waiting for page loaded.", "fileName": "resource://mozmill/modules/utils.js", "name": "TimeoutError", "lineNumber": 429}}
> TEST-UNEXPECTED-FAIL | /home/johns/moz/areweslimyet/cpg_breakage.js | cpg_breakage.js::setupModule

Comment 1

[Bobby Holley (:bholley)]

(In reply to John Schoenick [:johns] from comment #0)
> Created attachment 620526 [details]
> Very simple mozmill test that tries to load a page then calls
> c.waitForPageLoad()
> 
> Using Mozmill 1.5.11 and the upcoming compartment-per-global patch causes
> tests to fail on waitForPageLoad() calls.
> 
> CPG is bug 650353 - example try push with working CPG binaries at:
> https://tbpl.mozilla.org/?tree=Try&rev=35de20b7eae9
> 
> It's not clear if this is CPG breakage or Mozmill breakage. Mozmill appears
> to stick a 'mozmillDocumentLoaded' property on document.defaultView that is
> then read through jsbridge, so there could be a number of interactions.

If I understand you correctly, that's almost certainly the issue here. Expandos on content Xraywrappers aren't readable cross-compartment, and we're probably getting two chrome compartments here where we previously had only one.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Thunderbird is also running into this, which is unfortunate, because all of our UI tests are written in Mozmill, and so we're perma-oranged all over the place.

Cc'ing Henrik who (I believe) is still working on Mozmill.

Comment 3

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

This is my next action item for investigation and hopefully coming up with a fix by early next week. When is that feature going to land? Means how much time do we have to get out a fix?

Absolutely sounds like a blocker to me.

Comment 4

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

(In reply to Bobby Holley (:bholley) from comment #1)
> If I understand you correctly, that's almost certainly the issue here.
> Expandos on content Xraywrappers aren't readable cross-compartment, and
> we're probably getting two chrome compartments here where we previously had
> only one.

Not sure if I understand you correctly, but do you mean that Mozmill and JSBridge live in two separate compartments now and can't communicate to each other? 

But the 'mozmillDocumentLoaded' property set on the defaultView is only used by the Mozmill extension itself. JSBridge doesn't access this property.

I will have a quick check now but can't promise to come up with something before Monday. Sorry, but that information hit us by surprise. It would have been nice to get informed earlier. Once more a downside of not having Mozmill on buildbot. :/

Comment 5

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

Thanks to Mike I got more insight into what expandos are and how I can map this to what Mozmill is doing. Given the investigation with Clint, and Mike the issue is located in Mozmill. There is no fallout in the GPC patch.

The waitForPageLoad method in Mozmill polls the custom property mozmillDocumentLoaded which gets attached to any content window by event listeners. Given that we cross compartments from chrome to content, and waitForPageLoad can't find the property.

So one idea I have is that we completely get rid of this behavior. Modifying the DOM and attaching custom properties or attributes is always bad. Instead I would propose that Mozmill itself keeps a list of the window load states in a hash, so that it can easily access unique loaded states.

It would look like: 

{window1: { loaded: true}},
{window2: { loaded: false }},
[..]

The reason why I propose a nested object as value is that we already had the idea to store other data too, but weren't able to yet. In this case we could easily expand the dictionary to whatever we have to store for each individual window.

Clint, what do you think about this structure?

Comment 6

[cmtalbert]

From IRC:
14:27 < ctalbert> yep
14:27 < ctalbert> I like it
14:28 < ctalbert> The only thing I worry is that we will hang on to all open window objects until 
                  we shut down and destroy this hash
14:28 < ctalbert> but I don't think that will be a problem
14:28 < ctalbert> I'd say we go for it
14:28 < ctalbert> and deal with the leaks *if* they arise

Comment 7

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

Not necessarily. We already listen for unload events. So we could use those to get rid of the appropriate hash entries.

As talked on IRC and agreed on the hash should stay in controller.js for Mozmill 1.5 and probably driver.js in Mozmill 2.0.

Comment 8

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

(In reply to Henrik Skupin (:whimboo) from comment #7)
> Not necessarily. We already listen for unload events. So we could use those
> to get rid of the appropriate hash entries.

And just one additional note. This has to happen *before* the window is closed. Otherwise we would access an already dead object. See bug 751242 for an example.

Comment 9

[cmtalbert]

So, I've looked at this quite a bit tonight.  I don't think it's a simple drop in replacement. Harth & Jhammel & dburns & davehunt, I'd love to have your insight into this as well.

Here's why. Note: I'm looking at this in terms of the 1.5.x flow, we'll need to look in detail at the 2.x flow as a separate case, but I imagine the solution will be similar.

So the goal here is to create the object that Henrik proposes in comment 5, but persist said object among all possible controller objects that a test may instantiate.

So, you have to look at how controller objects are acquired and how the current "object" that is referenced by comment 5 is created.

Currently, the "object" isn't really an object, we add a property on every content DOM window that comes in and then each controller object (in its waitFor method) queries that property to see whether or not the window has loaded.  That's going to break with this compartment change because that chrome <--> content access isn't allowed.  That's also why this action has been an Achilles heel of Mozmill for some time -- we are running in the face of our own security model.

So, to solve this issue, I'm thinking about the following. 

The init.js file currently uses the window listeners to add the DOM property to each content window. We change the init.js file to create the hash proposed in  comment 5 rather than add the property to the content window objects. Let's call this object the "windowmap". 

Each time a window is loaded or unloaded, the observers in init.js fire, and the windowmap object is appropriately modified (adding or removing said window) and then the init.js function *replaces the mozmill.windowmap object* with the new windowmap.  That means that while init.js modifies the windowmap object, the mozmill.js object maintains the real object of record.

Then when some test calls mozmill.getBrowserController() (or *whatever* controller) the windowmap object is passed into the constructor of the controller.MozMillController object.  Because each test creates its own controller - note the "new controller.MozMillController" call in mozmill.js::getBrowserController and related APIs. By passing in the windowmap object to each controller, each controller for each test obtains a proper reference to the windowmap object and the controller.waitFor functions will work as they do now.

Since Javascript usually uses references in this case, this means that the windowmap in any given controller will always be modified as the window structure changes so it will maintain integrity as windows are created and destroyed in a test.

This should work, but I haven't been able to test it.  It's taken me a while to figure out how to go about the doing of this.  I'll try to implement tomorrow if Henrik doesn't beat me to it.

Here's why I think this will work, given the Javascript file:

function controller(windowmap){
  this.windowmap = windowmap;
}

var controller.prototype = {
  printit: function(){
     dump(windowmap.mywin + "\n");
  },
};
var wm = {'mywin': 3};
var mycontroller = new controller(wm);
dump("first: ");
mycontroller.printit();
wm = {'mywin': 4};
dump("second: ");
mycontroller.printit();

--------
when run you get:
first: 3
second: 4

So, if you imagine the window object having a set of values at the time the controller object for the test is specified, then the test opening a new window (causing the windowmap object to be modified) then the next time the controller accesses the windowmap object, it will see the modified version since it is querying the reference of the object.

I think this will work. /me crosses fingers. If anyone has any thoughts on it, I'm keen to hear them.

Comment 10

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

(In reply to Clint Talbert ( :ctalbert ) from comment #9)
> Then when some test calls mozmill.getBrowserController() (or *whatever*
> controller) the windowmap object is passed into the constructor of the
> controller.MozMillController object.  Because each test creates its own
> controller - note the "new controller.MozMillController" call in
> mozmill.js::getBrowserController and related APIs. By passing in the

Hi Clint. Everything sounds great but I have mixed feelings about this part. I don't really see the reason why we have to expose the windowMap to the test itself. Personally I don't want that getBrowserController gets an additional argument. Also I think it's not necessary. All the data to handle should happen behind the scenes. There is no need to have a reference of it in each controller. A single global object in controller.js should be enough. In that case we only have to import controller.js into init.js for its access. waitForPageLoad() which is only present in controller.js has ultimate access. Otherwise we would also have to import mozmill.js. Not sure how this would work because mozmill.js already imports controller.js. I don't want that we end up in an endless import loop. Why do you think it's important to have the windowMap in mozmill.js and not controller.js/driver.js?

I don't have time to work on it since around 8pm my time which is in about 12h. But if I do I will let you know. So please be around on IRC when you come online, and we can sync up.

If you have time to respond to the this comment before you head to bed, please do so. Thanks.

Comment 11

[cmtalbert]

No you misunderstand. I agree this should be done behind the scenes and not exposed to tests. I think this solution does that. The window map would not be an argument to mozmill.getBrowserController(). It would be an argument to the controller constructor that mozmill.getBrowserController creates. That way you don't change how controllers are constructed the window map stays behind the scenes.and tests never know it exists. I don't think it is even possible for a test to construct a controller directly so this should be pretty safe (all controller creation happens through the mozmill object). I'm on my phone so I can't verify but I think this is the Case. If you're still confused,  I'll take a stab at it and see what I run into.
Happy Saturday and good night! 

PS fennec sux for typing into this text box. Had to use stock android :( if someone has time to file a bug I'd be grateful.Otheewise this note will remind me to do so later

Comment 12

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

I still don't see a reason why we have to pass the map to each controller. It's not necessary. It's enough to have the global object which also gets accessed by the event handlers in init.js. Whatever controller instance a test works with, it's waitfForPageLoad method will have access to the global map.

Also we do not only have to handle browser windows. There are a lot of other types of windows a controller has to be created. Therefore the tests have to directly create the controller via MozMillController(). So using the map as parameter for this method would expose this map to tests.

Comment 13

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

This approach seems to work fine for me. I have made the changes I had in mind and it was kinda easy. Lesser work as I have expected. It will require further testing for all kinds of windows. It's something I will do next. For now I just want to show what I have so far, and which in fact is far from being complete. Right now it's just a PoC and misses:

* value is a hash too
* windows have to be removed from map if they are closed
* error detection and handling
* tests
* others....

The current code can be found here:
https://github.com/whimboo/mozmill/compare/mozautomation:hotfix-1.5...whimboo:gpc

I will find some more time later today (in about 2h) when I will continue with the patch.

Comment 14

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

So it's a tough thing to do. I have updated the branch on my github repo with the following changes:

* It's now using a new object to handle the map.
* Entries are unique window ids (currentInnerWindowID)
* The value is a hash too and holds all the set properties e.g. loaded state

Still to do or unclear:
* It doesn't work for preference dialogs e.g. testAwesomeBar/testCheckItemHighlight.js. Not tested with other modal dialogs yet
* Not sure when we should remove the entry from the map given that with session restore you can reopen a closed window. I have to check if it's the same id or not.

Comment 15

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

(In reply to Henrik Skupin (:whimboo) from comment #14)
> * It doesn't work for preference dialogs e.g.
> testAwesomeBar/testCheckItemHighlight.js. Not tested with other modal
> dialogs yet

Silly mistake on my side when I left an mozmillDocumentLoaded line in the code. It works fine now.

Comment 16

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

Attachment: WindowMap for page load handling (v1)

Pointer to Github pull-request

Comment 17

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

Comment on attachment 621352 [details]
WindowMap for page load handling (v1)

Works like a charm for me. Please let me know what you think. Further it would be nice to know how we want to handle closed tabs/windows. Do we want to remove those entries or leave them for now? I'm open both ways since we do not stick special properties on the map yet.

Comment 18

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

Mike or Mark, can one of you please show me some code how you handle tabs in Thunderbird? I kinda would like to get this integrated in this patch.

Comment 19

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

I have updated the patch again with the following changes:

* Move from currentInnerWindowID to outerWindowID because that's what we are really interested in.
* Add observer for when windows get closed/destroyed and remove items from the list.

I left in some debug output for now. Once we got the tab handling fixed for Thunderbird I will remove this too.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

I tried the patch that Henrik has up on Github, and I was able to call into windowMap to perpetuate the bit of hackery we were doing to make our content tab tests work.

So if that patch lands, TB is also in the clear.

Comment 21

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

Thanks Mike! I have finished up the patch and it is now ready for review. I will flag the request as necessary. I hope we can get this in at latest tomorrow morning European time.

Comment 22

[Mark Banner (:standard8)]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Mike or Mark, can one of you please show me some code how you handle tabs in
> Thunderbird? I kinda would like to get this integrated in this patch.

http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-content-tab-helpers.js

Comment 23

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

Mark, thank you for the link. But after talking with Mike yesterday I came to the conclusion that our new windowMap object is perfect to use even for other applications, which have their own window handling code. It will be hard to adapt Mozmill to all these various types. So if you have to handle a window register a listener and update/remove the entry in windowMap by calling the official API. I will make sure to add some documentation for it on MDN.

Patch on the hotfix-1.5 branch has been merged:
https://github.com/mozautomation/mozmill/compare/85d3aef...7e86670

I will follow-up with a patch for Mozmill 2.0 later.

Comment 24

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

Attachment: Patch (master)

Pointer to Github pull-request

Comment 25

[cmtalbert]

Needed to land on master to unblock peptest fixes. Pushed. https://github.com/mozautomation/mozmill/commit/1180b8fd23488a8c2b4ae40ba20528a3b7bae7a1

Comment 26

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

Thanks Clint for landing the patch.