634815 - Fix for bug 560008 breaks ping

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: Documentation, defect)

Description

[Will Bamberg [:wbamberg]]

The changes made to main.js for bug 560008 break pinging, so when running the self-hosted docs server using cfx docs the docs server eventually shuts down.

This should block the 1.0b3 release.

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: Fix ping

This patch fixes the jQuery.ajax call, meaning that ping request get sent, and stops sending them if the server responds with 404 or 501 (which imply that a server is serving static docs, so ping is not needed).

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 513030 [details] [diff] [review]
Fix ping

diff --git a/static-files/js/main.js b/static-files/js/main.js

+  server_needs_keepalive=true;

This needs to be declared with var/let; otherwise it becomes a global variable (or throws an exception if we ever turn on ES5 strict mode for this code).

Also, nits: the convention is to use camelCase for such values and put spaces around the equals sign, i.e.:

  var serverNeedsKeepalive = true;


                error: function(req) {
                  if (req.status == 501 || req.status == 404)
                    // The server either isn't implementing idle, or
                    // we're being served from static files; just bail
                    // and stop pinging this API endpoint.
+                   server_needs_keepalive = false;
                    return;

It's hard to figure out what is inside the if statement here, given that there are three lines of commentary between the conditional and the assignment, and the next statement (`return;`) is indented as if it too were inside the if statement.

One solution to this problem is to add curly braces around the assignment and its commentary:

  if (req.status == 501 || req.status == 404) {
    // The server either isn't implementing idle, or
    // we're being served from static files; just bail
    // and stop pinging this API endpoint.
    server_needs_keepalive = false;
  }

Another is to move the commentary above the if statement:

  // The server either isn't implementing idle, or
  // we're being served from static files; just bail
  // and stop pinging this API endpoint.
  if (req.status == 501 || req.status == 404)
    server_needs_keepalive = false;

Also, "return" is unnecessary, as this function no longer needs to return early.


Finally, it's unclear from looking at the existing code why it fails.  Can you elaborate on the nature of the bug?

Comment 3

[Will Bamberg [:wbamberg]]

> Finally, it's unclear from looking at the existing code why it fails.  Can you
> elaborate on the nature of the bug?

The existing code fails with 'jQuery.ajax is not a function': I don't quite understand why, as I would have expected jQuery to be defined at the time the function executes. The original code has the ping function inside startApp, which is called in $(window).ready.

Comment 4

[Will Bamberg [:wbamberg]]

(In reply to comment #3)
> > Finally, it's unclear from looking at the existing code why it fails.  Can you
> > elaborate on the nature of the bug?
> 
> The existing code fails with 'jQuery.ajax is not a function': I don't quite
> understand why, as I would have expected jQuery to be defined at the time the
> function executes. The original code has the ping function inside startApp,
> which is called in $(window).ready.

By the way: can you reproduce this error? I can in one environment, not in the other. Which at least explains why I didn't find it before.

Comment 5

[Will Bamberg [:wbamberg]]

Attachment: A very small add-on which reproduces the jQuery problem

> By the way: can you reproduce this error? I can in one environment, not in the
> other. Which at least explains why I didn't find it before.

It's something to do with having the annotator add-on installed as well. With the annotator installed, I get "jQuery.ajax is not a function", so no pings get sent and the server shuts down after a minute or so. Without it, the pings get sent and the server stays alive. 

But it's not specifically the annotator: I've cut it down to a pretty minimal add-on using page-mod and jQuery, which reproduces the problem. Would it be possible for you to check if you get it too? Is there anything obviously wrong in this minimal program which could cause this error?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 513181 [details]
A very small add-on which reproduces the jQuery problem

Hmm, yes, I can reproduce the problem with this addon installed, which is bizarre and disturbing.  We'll need to investigate that, but in any case, I'm ok taking a fix for the docs in the meantime.

Comment 7

[Will Bamberg [:wbamberg]]

Attachment: Updated patch

Yes, bizarre and disturbing. Here's another patch that should be more sensible.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

https://github.com/mozilla/addon-sdk/commit/a6fb272dc910664525b7f6c0babdd48a5ce86bb2