472132 - While recorder is running no additional windows can be opened and running the test results in an error ('$ is not defined')

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090104 Shiretoko/3.1b3pre ID:20090104034401 and MozMill 1.0rc3

There are a lot of tests where handling of different windows is necessary. That means while recording a session other windows have to be opened. Currently nothing happens. Running the test against the "Organize Bookmarks" menu entry gives following result:

var mozmill = {}; Components.utils.import('resource://mozmill/modules/mozmill.js', mozmill);
var elementslib = {}; Components.utils.import('resource://mozmill/modules/elementslib.js', elementslib);

var setupModule = function(module) {
  controller = mozmill.getBrowserController();
  controller2 = new mozmill.controller.MozMillController(mozmill.utils.getWindowByTitle("undefined"));
}

var testRecorded = function () {
  controller.click(new elementslib.ID(controller.window.document, "history-menu"));
  controller.click(new elementslib.ID(controller.window.document, "bookmarksShowAll"));
  controller2.click(new elementslib.ID(controller2.window.document, "statusMessage"));
  controller2.click(new elementslib.ID(controller2.window.document, "recorder"));
}

Running this test results in an error:

Error: $ is not defined
Source File: chrome://mozmill/content/output.js
Line: 99

Source:

// Set UI Listeners in frame
function stateListener (state) {
  if (state != 'test') {  
=>  $('runningStatus').textContent = state;
    // results.write(state)
  }
}

Comment 1

[Adam Christian]

Changing $ to document.getElementById, checked into trunk. Please verify.

Comment 2

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

Adam, the $ problem is fixed now. But recording a session where I want to open another window like the Library isn't possible at the moment. The main menu entry (here Bookmarks) stays highlighted but the Library isn't opened until you stop the recorder.

Comment 3

[cmtalbert]

I think that we have an issue here: (rec.js):
observe: function(subject,topic,data){
controller.waitForEval("subject.document.documentElement.getAttribute('windowtype') != null", 10000, 10, subject)
     var wtype = subject.document.documentElement.getAttribute('windowtype');
     if (wtype == "navigator:browser"){
       controller.waitForEval("subject.document.body != null", 10000, 10, subject)
     }
     //Attach listener to new window here
     MozMillrec.bindListeners(subject);
     }

The waitForEval seems to be timing out, and wtype is still null.  I think this is occuring because the window does not load due to the waitForEval call.  Unfortunately we are in a catch 22 here.  We can't attach to the window because we don't know what it is, and we don't know what it is, because it hasn't loaded.  Hmmmm...

Comment 4

[cmtalbert]

Attachment: A hacky patch that seems to fix the issue

Ok, so this patch appears to fix the issue.  I can't really explain why, it was merely a hunch I had that it would work.  There is a huge timeout while waiting for the new dialog to open, but if you wait it shows up.  I'm not sure if this indicates that the waitForEval is unneeded, or if this indicates that my machine is very very slow (it's building fennec at the moment, and everything is slow).

Please test and see what you think.

Comment 5

[Mikeal Rogers [:mikeal] (mrogers@)]

There is a wait for documentLoaded before the windowtype call. That shouldn't be firing so early that we break the document by asking for it's windowtype.

You can't use a defer like in this patch because the rest of the framework is libel to run a controller test that uses a windowtype specific method before 500 milleseconds is up.

Comment 6

[Adam Christian]

So here is my working replacement for all this:

//When a new win dom window gets opened
RecorderConnector.prototype.observer = {
  observe: function(subject,topic,data){
    var defer = function(){
      controller.waitForEval("subject.documentLoaded == true", 10000, 100, subject)
      MozMillrec.bindListeners(subject);
    }
    window.setTimeout(defer, 500);
  }
};

It turns out that now that we have the documentLoaded, we don't need to know the window type, or access/wait for the body. Turns out there was a few ms where the thread was getting stopped opening the window, and by waiting 500ms to start the wait we have provided enough time to start the opening of the window which appears to kick off its own thread and is then independent from the code doing the wait. I have tested this extensively and found it to be super accurate. 

Please verify.

Comment 7

[cmtalbert]

This patch works a lot better than mine did.  What about Mikeal's assertion that the rest of the framework might get in there before you return in this call?

Comment 8

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

Ok, the window can be opened now too. But the recorded test is still useless. Seems like we lose the thread here?

Comment 9

[Mikeal Rogers [:mikeal] (mrogers@)]

(In reply to comment #6)
> So here is my working replacement for all this:
> 
> //When a new win dom window gets opened
> RecorderConnector.prototype.observer = {
>   observe: function(subject,topic,data){
>     var defer = function(){
>       controller.waitForEval("subject.documentLoaded == true", 10000, 100,
> subject)
>       MozMillrec.bindListeners(subject);
>     }
>     window.setTimeout(defer, 500);
>   }
> };
> 
> It turns out that now that we have the documentLoaded, we don't need to know
> the window type, or access/wait for the body. Turns out there was a few ms
> where the thread was getting stopped opening the window, and by waiting 500ms
> to start the wait we have provided enough time to start the opening of the
> window which appears to kick off its own thread and is then independent from
> the code doing the wait. I have tested this extensively and found it to be
> super accurate. 
> 
> Please verify.

If you defer then the creation of the controller returns before the windowtype specific methods are added and possible before the window is ready for testing. 500ms is way too long.

We do need to know the windowtype by the way, that's what we use to add windowtype specific methods dynamically to the controller. We CANNOT return from the controller creation until those methods are added.

If there is some time where the thread gets stopped from creating the window maybe we can fix it with just a 10ms at the beginning, since even a 10ms sleep will call the main thread's processNext?

Comment 10

[Adam Christian]

500ms could probably be reduced down to like 5 or 10ms but the controller shouldn't be broken by this because it's instantiation waits for documentLoaded to be true which it won't be until this window is created and then the document is finished loading. You also have to remember that this is just happening when the recorder is on, when a test is running and the recorder is off this observer code isn't even touched. 

The generated test won't work because we also need to detect when these new
windows open and create a new controller for them but that is a different bug
than not being able to open new windows at all while recording is happening. I
submit that we close this and open a new one for correcting the generated test
to reflect the new windows.

Comment 11

[Mikeal Rogers [:mikeal] (mrogers@)]

Dammit, i totally combined this bug and another bug in my head. Too much looking at bugzilla lately.

I thought this was about the controller creation blocking calls.

Nevermind all my comments today.

Comment 12

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

Ok, now it's possible to open new windows while recording a session. But there are still some issues left. I'll file them as separate bugs. Marking this one as fixed for now.