Ubuntu CI Services

Merge lp://staging/~doanac/ubuntu-ci-services-itself/webui-status-performance into lp://staging/ubuntu-ci-services-itself

webui-status-performance
Merge into trunk

Proposed by Andy Doan on 2014-03-05

Status:	Merged
Approved by:	Andy Doan on 2014-03-05
Approved revision:	311
Merged at revision:	322
Proposed branch:	lp://staging/~doanac/ubuntu-ci-services-itself/webui-status-performance
Merge into:	lp://staging/ubuntu-ci-services-itself
Prerequisite:	lp://staging/~doanac/ubuntu-ci-services-itself/json-status-styling
Diff against target:	86 lines (+31/-24) 2 files modified webui/webui_status.js (+22/-24) webui/webui_status_worker.js (+9/-0)
To merge this branch:	bzr merge lp://staging/~doanac/ubuntu-ci-services-itself/webui-status-performance
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
William Grant	browser ajax hell	2014-03-05	Approve on 2014-03-05
PS Jenkins bot (community)	continuous-integration		Approve on 2014-03-05
Canonical CI Engineering		2014-03-05	Pending
Review via email: mp+209366@code.staging.launchpad.net

Commit message

webui: improve performance of status page

I noticed a tricky problem. If one of the status URLs we are trying
to reach is taking a really long time, the rest of the json URLs we
try to load get delayed on the main javascript thread.

This changes the code to use JavaScript workers which essentially do
the status loading in separate threads and then communicate back to
the main gui thread the status to display.

Description of the change

I noticed a tricky issue in the new json status page in the webui. Basically one of my hpcloud nodes didn't have proper permission for port 8080. This causes the loading of that port to take forever before it fails. The way (at least firefox) was handling the jsonp loading meant that each url we tried to load after the bad URL would be blocked in a "loading..." state.

This change uses Web Workers to do the loading in another thread and then communicate the status back to the main UI thread. I wanted to get this fixed before moving to a DataTables implementation so that we could make sure we start with the correct workflow on it.

Also of note: YUI is a pain to load into a web worker, so I took the simplified approach of just calling "importScripts" to get our JSONP data.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-05:

PASSED: Continuous integration, rev:311
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/296/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/296/rebuild

review: Approve (continuous-integration)

Revision history for this message

William Grant (wgrant) wrote on 2014-03-05:

This web worker approach looks fine, assuming we're OK with the browser restrictions (most notably the lack of support in IE <10 and the Android stock browser before 4.4). The JSONP requests should be to a variety of domains, so we won't run into per-origin concurrent request limits.

Separately, I'm a little concerned about some of the DOM construction mechanisms in use here.

48 + if(e.data.error) {
49 + sts.innerHTML = 'ERROR: ' + e.data.error;
50 + return;
51 + }

This will render a textual error message as if it was HTML. Users can often control error content (eg. if a method includes the bad input in an error message about invalid arguments), and setting innerHTML will cause markup to be parsed -- a cross-site scripting vulnerability.

53 + var txt = "<pre>";
54 + for (i in resp) {
55 + if(resp[i]['status'] != 'okay') {
56 + txt += resp[i]['status'].toUpperCase() + ': ';
57 }
58 + txt += resp[i]['label'] + ": " + resp[i]['value'] + "\n";
59 }
60 - });
61 + txt += "</pre>";
62 + sts.innerHTML = txt;

Likewise here. If any of the status, label or value fields are user-controlled, this is an XSS vulnerability. Or if some data happens to contain <, & or other metacharacters you'll end up misrendering.

YUI3 provides reasonably nice APIs for safe DOM construction. We often use a pattern like this:

  var div = Y.Node.create("<tr><th></th><td><span></span><a></a></td></tr>");
  div.one("th").set("text", "Header");
  div.one("span").set("text", "Label:");
  div.one("a").set("text", "Value").set("href", "http://url.to/whatever");
  parent.append(div)

For more complex structures, it's often cleaner to use a templating language like mustache or handlebars. Handlebars is supported by YUI3 core, in http://yuilibrary.com/yui/docs/handlebars/.

review: Approve (browser ajax hell)

Revision history for this message

Andy Doan (doanac) wrote on 2014-03-05:

On 03/05/2014 07:13 AM, William Grant wrote:
> Separately, I'm a little concerned about some of the DOM construction mechanisms in use here.

ack. i'm actually working up a separate MP for that. I'm using YUI
DataTables locally and its cleaned things up quite a bit.

Revision history for this message

Andy Doan (doanac) wrote on 2014-03-05:

Will - you might want to look at:

https://code.launchpad.net/~doanac/ubuntu-ci-services-itself/json-datatables/+merge/209506