Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#16386 closed defect (fixed)

race condition executing tests before initRobot() completes

Reported by: bill Owned by: bill
Priority: undecided Milestone: 1.7.6
Component: TestFramework Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

Editor_mouse.html and other tests have code like:

doh.robot.initRobot('../test_Editor.html');

doh.register("setup", [
        {
                name: "wait for editors to load",
                timeout: 5000,
                runTest: function(){
                        return new dojo.DeferredList(
                                dijit.registry.filter(function(w){ return w.onLoadDeferred; }).map(function(w){ return w.onLoadDeferred; })
                        );
                }
        }
]);

The "setup" test group is meant to run after the initRobot() call finishes loading the specified URL, and that page finishes initializing, in particular after the parser finishes running. However, it's running too soon.

dojo/robotx.js has code to try to pause until the iframe finishes loading:

var onIframeLoad = function(){
        // initial load handler: update the document and start the tests
        robot._updateDocument();
        onIframeLoad = null;

        // If dojo is present in the test case, then at least make a best effort to wait for it to load.
        // The test must handle other race conditions like initial data queries by itself.
        if(iframe.contentWindow.require){
                iframe.contentWindow.require(["dojo/ready"], function(ready){
                        debugger;
                        ready(999, function(){
                                robot._run(robotFrame);
                        });
                });
        }else{
                robot._run(robotFrame);
        }
};

However, robot._run() is also called from the _onKeyboard handler:

robot._onKeyboard = function(){
        // called from Robot
        // Robot calls _onKeyboard after it finishes discovering the keyboard
        // need to untie from Java thread with setTimeout
        setTimeout(function(){
                rf.style.visibility = "hidden";
                robot._run(window.frameElement);
        }, 0);
};

... so this starts running the "setup" test before the iframe has finished loading.

It's confusing to have two separate calls to robot._run(). It seems like the initRobot() should work more like any other registerd test, returning a Deferred that doesn't execute until the iframe has finished loading.

Change History (12)

comment:1 Changed 7 years ago by bill

Milestone: tbd1.9
Owner: set to bill
Status: newassigned

Note that the problem only shows up for me on chrome/win.

comment:2 Changed 7 years ago by bill

In [30057]:

Make sure that domReady(), when used as a function rather than a plugin, executes registered callbacks sequentially. This is how ready() is supposed to work, so it makes sense for domReady() to follow suit.

Plus, it's needed to make ready() work as advertised, since ready() calls domReady() internally. Note that it's still dodgy that there are two queues, one in dojo/ready and one in dojo/domReady. However, since dojo/ready is essentially deprecated, that's probably not worth fixing.

This fix is mainly a recursive guard, i.e. if f1() calls domReady(f2), f2() will be delayed until f1() completes. However, it also addresses a concurrency issue:

In Editor_mouse.html on chrome I noticed that DOH started running the tests while test_Editor.s dojo.ready() callback was still executing. It was mainly because the ready(999, ...) call from DOH occurred while the test_Editor.html dojo.ready() callback was running. Normally browsers don't run multiple threads but I guess this is an exception since DOH and test_Editor.html are in different frames.

Also added a try/catch around each domReady() callback so that a failure in one callback won't affect other code, and won't get the guard variable stuck.

Fixes #16389, refs #16386 !strict.

comment:3 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [30060]:

Refactor robot so initRobot() is implemented as an asynchronous test fixture, rather than using monkey patching code that doesn't work well, having two places calling doh._run() in a race condition. This design also makes better code separation between vanilla DOH and robot.

I plan to do the same for startRobot() in the next check in.

Also fixes priority of ready() call inside DOH. The default priority is 1000, so we need to be higher than that, so that DOH doesn't execute until the iframe's dojo.ready() call has finished executing.

Fixes #16386 !strict.

comment:4 Changed 7 years ago by bill

In [30074]:

Refactor robot so startRobot() is implemented as an asynchronous test fixture, rather than monkey patching doh.run(). This design makes better code separation between vanilla DOH and robot.

Refs #16386 !strict.

comment:5 Changed 6 years ago by bill

In [31072]:

avoid spurious timeouts starting robot, refs #16386 !strict.

comment:6 Changed 6 years ago by mahays0 <mahays0@…>

In 57cacb66b7ed89c4a9ca2eb4ec7587fb95ef1ab5/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:7 Changed 6 years ago by mahays0 <mahays0@…>

In c52544a0e0ede89178bb3393cb4e737baf0026ca/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:8 Changed 6 years ago by bill

Milestone: 1.91.8.6

comment:9 Changed 5 years ago by bill

Note: [31072] also backported to 1.8:

commit b2ea6d9a81424d16405f0d7466a926906f8f3d64
Author: Bill Keese <bill@dojotoolkit.org>
Date:   Mon Apr 1 08:11:45 2013 +0000

    avoid spurious timeouts starting robot, refs #16386 !strict.
    
    git-svn-id: http://svn.dojotoolkit.org/src/util/trunk@31072 560b804f-0ae3-0310-86f3-f6aa0a117693
    (cherry picked from commit 15e6c812122d35cbf49db93a5e79a81f8758d206)

comment:10 Changed 5 years ago by Bill Keese <bill@…>

In 1995dc8a70f1aee19a215bdeef9156eed9ae0ed5/util:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:11 Changed 5 years ago by bill

Milestone: 1.8.61.7.6

comment:12 Changed 4 years ago by Bill Keese <bill@…>

In a79c2dad1c169bf32cdbde6b741d4baf6170367b/util:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.