591698 - nsDOMWorkerFunctions returning false without setting error

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Luke Wagner [:luke]]

Perhaps we are missing something, but Waldo pointed out several cases where natives called from JS appear to be returning false without setting an error.  Without an error set, this will cause a silent engine exit.

As pointed out in bug 581263 comment 38, instances include:
nsDomWorkerFunctions::MakeTimeout
nsDomWorkerFunctions::KillTimeout
nsDomWorkerFunctions::LoadScripts
nsDOMWorkerFunctions::NewXMLHttpRequest
nsDOMWorkerFunctions::MakeNewWorker

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch (against 58218:0b53fd11e374)

This is me doing my best "wade in with a patch even though I barely know what I'm doing" routine.  I just looked in that file for every occurrence of a "return JS_FALSE" without a corresponding "JS_ReportSomething()" and added one.  I also looked in other files in the same directory, this file looks like the only affected one.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 495435 [details] [diff] [review]
patch (against 58218:0b53fd11e374)

JSAPI functions (including the JS_THIS_OBJECT "function") report an error when they return false or NULL, as a general rule.  (There are some "querying" API methods that infallibly return true or false to answer the relevant question, but they're the exception to the rule, and they clearly indicate when they do this -- or should, file bugs or fix MDN documentation if you find cases.)

Thus you don't need to, and shouldn't, report an error if JS_THIS_OBJECT, JS_ValueToString, JS_GetStringEncodingLength, or JS_NewStringCopyN fails by returning false/NULL.  You also don't need, and shouldn't, comment instances where a JSAPI function reports an error itself and no extra report is needed.  That false/null-returning JSAPI functions report an error as a rule is a JSAPI idiom that JSAPI users learn quite early on when working with the JSAPI.

But do keep reporting if nsMemory::Alloc fails, because that's not a JSAPI function (and same with the IsCanceled calls).  It's a generalized memory allocation function (and IsCanceled is a clear infallible predicate), so it can't report an error and thus you should indeed report here.

Is this advice clear regarding how to cargo-cult a little less harder here?

Comment 3

[Nicholas Nethercote [inactive]]

Thanks for the explanation.  In the future I suggest you avoid the phrase "cargo-cult".  I'm sure you meant it in a humorous fashion but the phrase has negative connotations that aren't helpful to someone who's trying to help with a blocker in a part of the code with which they are unfamiliar.

Comment 4

[Jeff Walden [:Waldo]]

My apologies, didn't occur to me that it might be interpreted that way -- will do.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch v2 (58531:dadc1d60b6f3)

New patch, which only reports if isCanceled() or Alloc() fails.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 496027 [details] [diff] [review]
patch v2 (58531:dadc1d60b6f3)

Looks good.

Comment 7

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/49f6b73ae373

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/49f6b73ae373

Comment 9

[Damon Sicore (:damons)]

We're in the endgame of Firefox 4.  Please make sure we only work on blockers!  Maybe this should have blocked, but if not, we must stay focused and only work on the most important bugs:  Blockers.  This means only reviewing blockers, too, unless there are significant reasons to ignore this guideline like reviewing a patch from a new contributor to encourage continued participation.  Otherwise, we must stick to our guns and only review blockers.

Hold the line!

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> We're in the endgame of Firefox 4.  Please make sure we only work on blockers! 
> Maybe this should have blocked, but if not, we must stay focused and only work
> on the most important bugs:  Blockers.  This means only reviewing blockers,
> too, unless there are significant reasons to ignore this guideline like
> reviewing a patch from a new contributor to encourage continued participation. 
> Otherwise, we must stick to our guns and only review blockers.
> 
> Hold the line!

I thought this was marked as a blocker when I started working on it.  Maybe I was looking at the wrong list of bugs at the time.  Sorry.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Wait. This patch is wrong, and should be backed out!

We want silent termination in workers when the worker is canceled - the mechanics of cancelling a worker when the user navigates away from the page depend on it. The error reporter on DOM worker threads generates DOM events that propagate to the main thread, also. Please undo this.

And in the future please CC me on worker-related bugs :)

Comment 12

[Nicholas Nethercote [inactive]]

Backed out:
http://hg.mozilla.org/tracemonkey/rev/56df247f792a

Sorry for the breakage.

I guess this should be marked RESOLVED INVALID once Sayre merges to m-c.

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Let's add some comments there, because unreported error usually means mistake.

Comment 14

[Robert Sayre]

(In reply to comment #9)
> We're in the endgame of Firefox 4.  Please make sure we only work on blockers! 

I think jumping on one-liner error report patches falls into the realm of micromanagement. However, it is instructive that this patch introduced bugs!

 (In reply to comment #13)
> Let's add some comments there, because unreported error usually means mistake.

The code is poorly factored, since the unreported error occurs 10 times. 8 are identical, and so remaining two are also identical.

Better to just leave it and fix later, imho.

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to comment #14)
> 
> Better to just leave it and fix later, imho.

Yeah, the bug is still open.  Besides, I heard we're all blockers, all the time now.

Though I did wonder why there weren't any test failures on TBPL...

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #14)
> The code is poorly factored, since the unreported error occurs 10 times.

How would you refactor this exactly? The whole point is to throw an uncatchable exception as quickly as possible once the worker has been canceled, so we check at the beginning of each function.

> Better to just leave it and fix later, imho.

You mean to not back out this patch?

Comment 17

[Robert Sayre]

(In reply to comment #16)
> (In reply to comment #14)
> > The code is poorly factored, since the unreported error occurs 10 times.
> 
> How would you refactor this exactly? The whole point is to throw an uncatchable
> exception as quickly as possible once the worker has been canceled, so we check
> at the beginning of each function.

The functionality is correct, afaik. I noticed the organization issue because Shaver suggested writing a comment, and I looked at where such a comment might live. I don't think writing a comment ten times would be good. For example, look at how many times this block of code appears:

>   nsDOMWorker* worker = static_cast<nsDOMWorker*>(JS_GetContextPrivate(aCx));
>   NS_ASSERTION(worker, "This should be set by the DOM thread service!");
> 
>   if (worker->IsCanceled()) {
>     JS_ReportError(aCx, "Worker is canceled");
>     return JS_FALSE;
>   }
> 
>   if (!aArgc) {
>     JS_ReportError(aCx, "Function requires at least 1 parameter");
>     return JS_FALSE;
>   }

Some functions don't need arguments. So one could write a CheckPreconditions() function accounting for that. Inspecting this code, it's not clear to me whether nsDOMWorkerFunctions::MakeTimeout needs to check its argument count. You probably know the answer, but it could be self-documenting. The out-of-memory checks also point to at least ten lines of code that are repeated twice:

>   size_t len = JS_GetStringEncodingLength(aCx, str);
>   if (len == size_t(-1))
>       return JS_FALSE;
> 
>   JSUint32 alloc_len = (len + 1) * sizeof(char);
>   char *buffer = static_cast<char *>(nsMemory::Alloc(alloc_len));
>-  if (!buffer)
>+  if (!buffer) {
>+      JS_ReportOutOfMemory(aCx);
>       return JS_FALSE;
>+  }
> 
>   JS_EncodeStringToBuffer(str, buffer, len);
>   buffer[len] = '\0';
> 
>   nsDependentCString string(buffer, len);
>   nsCAutoString result;

I think both of these cases could be written once rather than multiple times, but it is not an urgent issue given the blockers we face.

> 
> > Better to just leave it and fix later, imho.
> 
> You mean to not back out this patch?

No, I mean to skip adding a comment and leave the code as-is, with the backout.

- Rob

Comment 18

[Benjamin Smedberg]

Blocking on the backout, and then resolving this INVALID or whatever is appropriate.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #17)
> I think both of these cases could be written once rather than multiple times,
> but it is not an urgent issue given the blockers we face.

Oh, those are already fixed in tm, bug 612642.

Thanks for fixing this quickly Nicholas!

Comment 20

[Nicholas Nethercote [inactive]]

jst: you marked this as blocking betaN+.  Does that mean the back-out of the original patch is needed for betaN?  That's the only sensible interpretation I can make, because the original problem turned out to be a non-problem.

Comment 21

[Johnny Stenback (:jst)]

bsmedberg actually marked this blocking betaN+ (then bent accidentally undid that, and I restored it), and the backout already happened didn't it (comment 12, which got merged to m-c at some later point). I don't see anything left here that's blocking worthy, but please renominate if I'm missing something.

Comment 22

[Nicholas Nethercote [inactive]]

Marking this as RESOLVED INVALID, which I think is the right thing to do.  If it's not, please open a new bug;  this one's history is too confusing to continue on with.