465141 - Workers: Implement expression flavor of timeouts, use nsAutoJSValHolder

Status: RESOLVED FIXED

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

Description

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

Attachment: Patch, v1

Using expressions in timeouts is currently unimplemented (oops!) so here's a patch to do that. While I was here I yanked the manual rooting in favor of the nsAutoJSValHolder.

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 348403 [details] [diff] [review]
Patch, v1

- In nsDOMWorkerTimeout::ExpressionCallback::Run():

+  JSString* expression = JS_ValueToString(aCx, mExpression);
+  NS_ASSERTION(expression, "This shouldn't fail, verified in constructor!");

JS_ValueToString() can fail here due to OOM even if it didn't in the constructor, so you probably want to check here as well.

And this could use a trivial mochitest to make sure the expression flavor of timeouts do indeed work.

r+sr=jst with that.

Comment 2

[Johnny Stenback (:jst)]

Making this a blocker, w/o this we only have half of an implementation of timeouts in workers, which would be a really bad inconsistency for developers.

Comment 3

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/9d35d79ab1e9

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: bustage fix

I had to land this as bustage fix.  this patch made everything bug os x go red (I didn't want to backout all of bent's patches in the bundle if I could avoid it.

Comment 5

[Shawn Wilsher :sdwilsh]

Reopening to track the bustage fix.  Smaug looked at this on irc and though it looked OK.  It built on OS X before I pushed.

http://hg.mozilla.org/mozilla-central/rev/2505e91f4141

Comment 6

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

Attachment: Add OOM check to bustage fix

Applies on top of attachment 350388 [details] [diff] [review], just adds OOM check. Thanks sdwilsh!

Comment 7

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

Pushed changeset 411f1455e323 to mozilla-central.

Comment 8

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

Pushed changeset fa864682e05a to mozilla-1.9.1.