176182 - JS API users in Mozilla must follow the Request Model

Status: RESOLVED FIXED

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

Description

[Brendan Eich [:brendan]]

I'll have a patch tomorrow.  By enabling (and fixing compilation probs) the
JS_PARANOID_REQUEST/CHECK_REQUEST stuff in jsapi.c, I've found a bunch of calls
to ::JS_* functions that do not run in requests.  This could be a GC hazard some
day if the GC could run on another thread, or on multiple threads
(multi-threaded mark phase); and it could lead to deadlock today if an object
owned by the main thread were accessed on another thread.

To help, I'm introducing an #ifdef __cplusplus JSAutoRequest class to jsapi.h
(!). There's no better place, it's the right thing.

/be

Comment 1

[Brendan Eich [:brendan]]

Oops, js/src/xpconnect/src/xpcwrappednativeinfo.cpp's XPCNativeMember::Resolve
needs to run a request on the safe context it uses.

/be

Comment 2

[John Bandhauer]

My bad. I doubt there are other such cases in xpconnect. Easy enough to fix, of
course. I *love* the idea of an inlined AutoRequest C++ class in jsapi.h

Comment 3

[Brendan Eich [:brendan]]

I notice that xpconnect (and now the IDispatch based comconnect code) suspect
the request when calling via xpti into a "native".  I don't think that's
workable, it requires most dom natives to re-request again.  I think we need to
find the few (if any) blocking natives and suspend within their code, around
only the blocking call(s).

Jband, dbradley, you guys ok with this?  I could get rid of AutoJSSuspendRequest.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed jsapi.h extension for C++ sanity and convenience

I'm still struggling with the tangled maze of contexts and window objects in
embedding/components/windowwatcher/src/nsWindowWatcher.cpp, but I hope to have
the tree-wide patch soon.

/be

Comment 5

[David Bradley]

I'd probably privatize copy/assignment as well.

As far as Comment #3. I think there might be an issue with double wrapping but I
need to check, unless jband knows off the top of his head. Also, would there be
any plugin code this might affect?

Comment 6

[John Bandhauer]

Brendan, your JSAutoRequest class assumes that it is only used in an environment
where the request rules are strictly followed. In the past my request code was
always conditional on JS_GetContextThread to allow for cases where the
embedding's policy was not so strict (but still safe). Even if you enforce this
stictness in mozilla is it safe/polite to assume that js and xpconnect will
always be used in embeddings with similar strictness? You *could* add #ifdef'd
JS_GetContextThread checks to allow for those cases.

On the suspend issue... xpconnect calls out to *completely* arbitrary native
code. How could it not suspend in general? How about adding a classinfo flag to
indicate native classes that don't require or want the suspend call and setting
that flag in the dom classes? Though, DOM classes are not *supposed* to have to
know about JS and track such things as suspending requests! What exactly is your
goal on this front? Do you have data that shows mozilla suspending and
re-entering requests enough to degrade performance or something?

Comment 7

[Brendan Eich [:brendan]]

Next alpha.

/be

Comment 8

[José Jeria]

Will this be checked in now (1.4a)?

Comment 9

[Brendan Eich [:brendan]]

*** Bug 240462 has been marked as a duplicate of this bug. ***

Comment 10

[timeless]

Comment on attachment 103919 [details] [diff] [review]
proposed jsapi.h extension for C++ sanity and convenience

>>     void suspend() {
>>         mSaveDepth = ::JS_SuspendRequest(mContext);
>>     }
do you want to complain about people calling suspend repeatedly?

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 103919 [details] [diff] [review]
proposed jsapi.h extension for C++ sanity and convenience

(I personally dislike the :: prefix, but if you insist, shouldn't you apply it
consistently to all the JS names, such as JSContext and jsrefcount?)

r=shaver regardless, of course.

Comment 12

[Brendan Eich [:brendan]]

More work needed.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: updated jsapi.h patch

Removed those noisy ::s.

/be

Comment 14

[Brendan Eich [:brendan]]

I heard from biesi that timeless has a patch with lots of manual JS_BeginRequest
and JS_EndRequest calls hacked into xpconnect, dom, and content code.  That
should be a patch in a bug, and it ought to use JSAutoRequest where possible. 
timeless?

/be

Comment 15

[timeless]

a c++ object would certainly make my changes a lot cleaner. i'll try to pull
those changes (and not the other changes to the same files) into bugs this week
(ideally tomorrow, since i'll be out of town starting friday through the weekend).

Comment 16

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183611 [details] [diff] [review]
updated jsapi.h patch

r=shaver

Comment 17

[timeless]

Attachment: limited change set

this patch is a redacted version based only on the files whose changes i gave
to biesi a while ago. if i've added requests to other files, they'll have to be
caught at a later date. note there's at least one XXX about a related problem
and note that not all BeginRequests have end requests, JS_DestroyContext will
terminate the request (moving this to c++ will require a scope change or
something).

anyway. i'm off to catch my flight. i'll be back monday (albeit very burried),
so i'll catch up on stuff next thursday at the earliest.

Comment 18

[Brendan Eich [:brendan]]

timeless: have you thought about using JSAutoRequest, now that it's in the trunk jsapi.h?

/be

Comment 19

[Brendan Eich [:brendan]]

timeless: can you take this bug and put an up-to-date patch in it?  I'll review and if you need testing help, give the word.  It would be good to get this in.

/be

Comment 20

[timeless]

Attachment: updated patch

this patch is a working version. I had to extract it from a very very big changeset. I tested bugzilla, gmail, reader, domi, the debug xbl testsuite, tabbrowser, jsconsole, profile manager, help, xpcshell and opened venkman.

for my reference, this changeset includes changes to xpcjsruntime, xpcwrappednativeinfo, xpcshell, and most importantly nsxblprotoimplproperty (boolean negation logic is hard).

Comment 21

[timeless]

Comment on attachment 211360 [details] [diff] [review]
updated patch

Index: js/src/jsapi.c
@@ -92,6 +92,7 @@
+#define JS_PARANOID_REQUEST

This change is obviously not for committing to cvs, although it would be good if people testing would run with it. otherwise it's kinda hard to find out if i missed any spots.

Comment 22

[timeless]

Comment on attachment 211360 [details] [diff] [review]
updated patch

mozilla/allmakefiles.sh
mozilla/js/src/xpconnect/src/nsScriptError.cpp
mozilla/toolkit/content/widgets/dialog.xml
mozilla/toolkit/content/widgets/menulist.xml

none of these changes are from this patch, they just happened to be in the tree i was using. they're harmless.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 211360 [details] [diff] [review]
updated patch

Timeless: thanks for attaching.  I'm not looking for free labor from you, but I going to insist that someone (mrbkap, or me -- but expect delays) convert this patch to use JSAutoRequests.

Also, IIRC XPConnect suspends requests when calling out to a wrapped native's method, which means DOM natives will run outside of requests. Isn't that a big problem in general, since many of those natives call back into JS_*?  Or did you hit all such calls?  Hard to tell at a quick read-through, so I thought I would ask.

/be

Comment 24

[Brendan Eich [:brendan]]

In answer to my second paragraph in the last comment:

<timelyx> brendan: as long as i'd hit it in normal browsing, it was fixed
<timelyx> any time you reenter via xpconnect, you're covered by xpconnect
<timelyx> i think i actually looked for JS_ references in dom
<timelyx> and i certainly hit all the things in classinfo

Sounds good.  Just looking for auto-helpers.  Will make a deal with mrbkap tomorrow (trade E4X bug for this bug!).

/be

Comment 25

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

Comment on attachment 211360 [details] [diff] [review]
updated patch

dbaron, mrbkap, and timeless have all been really helpful in fingering this bug as the culprit behind one of Songbird's major crashes, so I'm working on this. I'll post the patch with the JSAutoRequest additions either later today or tomorrow.

Comment 26

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

Attachment: [NOT FOR CHECKIN (-w)] Request Model with JSAutoRequest (Trunk) v1.0

Wow, that took a LOT longer than I thought it would. In any case, here's the updated patch (ignoring whitespace changes for easier reading). I don't know a ton about which JS_ calls should and shouldn't be wrapped with the JSAutoRequest so I followed timeless' patch (attachment 211360 [details] [diff] [review]) pretty closely. There were a few cases where the previous request usage was a little inconsistent and so I tried to fix that up. I didn't include any of timeless' rooting changes as I figured that they were part of another patch. Also, the other patch had bitrotted so I upped the number of context lines in the patch in the hopes that this one won't.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 218264 [details] [diff] [review]
[NOT FOR CHECKIN (-w)] Request Model with JSAutoRequest (Trunk) v1.0

>Index: js/jsd/jsd_high.c
>@@ -128,26 +128,28 @@ _newJSDContext(JSRuntime*         jsrt, 
>+    JS_BeginRequest(jsdc->dumbContext);
>     jsdc->glob = JS_NewObject(jsdc->dumbContext, &global_class, NULL, NULL);
>     if( ! jsdc->glob )
>         goto label_newJSDContext_failure;
> 
>     if( ! JS_InitStandardClasses(jsdc->dumbContext, jsdc->glob) )
>         goto label_newJSDContext_failure;
>+    JS_EndRequest(jsdc->dumbContext);

You need to add a JS_EndRequest to the label_newJSDContext_failure case too, or you won't ever end this request.

>Index: js/src/js.c

C sucks ;-).

Looks good otherwise, r=mrbkap

Comment 28

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

Attachment: Request Model with JSAutoRequest (Trunk) v1.1

Updated patch with mrbkap's potential bug resolved. This patch is ready for checkin (i.e. whitespace changes are included).

Comment 29

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

Attachment: A list of JS API functions that should be used in conjunction with JSAutoRequest

Not sure where else to put this at the moment, so I'll attach it.

I wasn't realy sure which JS_ functions should be wrapped within requests, so at the advice of mrbkap I made a list of all the functions in jsapi.c that use the CHECK_REQUEST macro. For easy reference (or whatever).

Comment 30

[timeless]

Comment on attachment 218346 [details] [diff] [review]
Request Model with JSAutoRequest (Trunk) v1.1

>Index: js/src/xpconnect/shell/xpcshell.cpp
>+        JSAutoRequest ar(cx);

xpcshell should be treated as a C file, it's supposed to be kept in sync w/ the other jsshells. the changes you made to js.c should be used here too. Eventually someone will refactor them into a happy lib (i have a tree somewhere that tried to at least harmonize some of the rest of the code to help that process along).

wrt merging. http://viper.haque.net/~timeless/patchtools
- use mergePcvs and then diff to your heart's content, or cvs update to any random version.

btw, thanks a lot for adopting this. i'm unfortunately overloaded and will not have a chance to look at this patch seriously until the middle of may at the earliest. hopefully it'll be in cvs by then.

Comment 31

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

Attachment: Request Model with JSAutoRequest (Trunk) v1.2

Updated with timeless' comments on xpcshell.

BTW, timeless mentioned that there were other bugs on components that didn't follow the request model... I've searched and searched through bugzilla and I haven't found any more as of yet. Anyone know where they're hiding or have some magic search terms that I haven't thought of yet?

Comment 32

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

(In reply to comment #31)
> Anyone know where they're hiding or have some magic search terms that
> I haven't thought of yet?

Ok, timeless pointed me to some. I created bug 334032 to help me track them.

Comment 33

[Brendan Eich [:brendan]]

Comment on attachment 218264 [details] [diff] [review]
[NOT FOR CHECKIN (-w)] Request Model with JSAutoRequest (Trunk) v1.0

Should I review v1.2?  Please request me if so.  Thanks for doing this (if you are using Gecko from multiple threads, however, this won't save you -- we should tawk ;-).

/be

Comment 34

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

Comment on attachment 218442 [details] [diff] [review]
Request Model with JSAutoRequest (Trunk) v1.2

(In reply to comment #33)
> Should I review v1.2?  Please request me if so.

Sorry, I left that other request open because it was the -w version. v1.2 is the most up-to-date and incorporates both mrbkap's and timeless' comments, so yes, that is the one I'd like you to review. Thanks!

  Thanks for doing this (if you
> are using Gecko from multiple threads, however, this won't save you -- we
> should tawk ;-).

I was afraid of that... What other bugs need to be filed/fixed?

Comment 35

[Brendan Eich [:brendan]]

(In reply to comment #34)
> (From update of attachment 218442 [details] [diff] [review] [edit])
> (In reply to comment #33)
> > Should I review v1.2?  Please request me if so.
> 
> Sorry, I left that other request open because it was the -w version. v1.2 is
> the most up-to-date and incorporates both mrbkap's and timeless' comments, so
> yes, that is the one I'd like you to review. Thanks!

Ok.  Is it worth a new -w version for my eyes' sakes?

>   Thanks for doing this (if you
> > are using Gecko from multiple threads, however, this won't save you -- we
> > should tawk ;-).
> 
> I was afraid of that... What other bugs need to be filed/fixed?

They're yours to fix, I'm afraid.  Gecko is not multi-threaded and won't be without help from something like Oink (see bug 40848).  Why do you think you want to run Gecko code (not SpiderMonkey, XPConnect, or XPCOM code, but rather parser, DOM, layout, rendering, etc.) off the main thread?

/be

Comment 36

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

Attachment: [NOT FOR CHECKIN (-w)] Request Model with JSAutoRequest (Trunk) v1.2

Here's the -w version of the latest patch. Since a lot of the changes involved adding scoping braces I think it might make your life easier to read this one.

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 218442 [details] [diff] [review]
Request Model with JSAutoRequest (Trunk) v1.2

Recurrent pattern to fix:

    Type var = nullOrFalse;
    {
        JSAutoFrobber af;
        var = JS_TheRealDeal();
    }

The initialization is useless, and the narrow scoping of af is not necessary in short functions.  Eliminate both if possible, the first in any event.

Otherwise it looks good to me.  I'll bless a new patch.

/be

Comment 38

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

Attachment: Request Model with JSAutoRequest (Trunk) v1.3

>The initialization is useless, and the narrow scoping of af is not necessary
>in short functions.  Eliminate both if possible, the first in any event.

Done.

Comment 39

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

Attachment: [NOT FOR CHECKIN (-w)] Request Model with JSAutoRequest (Trunk) v1.3

Comment 40

[Brendan Eich [:brendan]]

Comment on attachment 218907 [details] [diff] [review]
Request Model with JSAutoRequest (Trunk) v1.3

I'm a little concerned about performance effects of this patch, but we can roll the dice and monitor the tinderboxes that test Tp, etc.  Anyone able to measure in advance of checkin and give reassurance, please do so.

Wrongful removal of braces around single-statement then clauses in nsJSEventListener::HandleEvent.

Gratuitous reformatting of scriptEvent initializer, with off-by-one continuation line indentation, in same method.

Gratuitous rewrite of nested if-then to switch with fall-through, reversing order of operations, in nsXMLHttpRequest::Open.

Gratuitous ok initialization in jsd_NewValue -- lose the = JS_FALSE initializer.

Please don't patch js.c, it is single-threaded code.  A trivial request in main suffices, and that change already landed in 3.108.

XPC_NW_NewResolve should use a single request, not two in a row in the case where the ::JS_DefineUCProperty call is reached.

It's close, thanks for taking this on.  I'll stamp the next patch, in all likelihood.

/be

Comment 41

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

Attachment: Request Model with JSAutoRequest (Trunk) v1.4

(In reply to comment #40)
> Wrongful removal of braces around single-statement then clauses in
> nsJSEventListener::HandleEvent.

Oops, I got a little too happy there. Added them back.

> Gratuitous ...

Fixed.

> Please don't patch js.c, it is single-threaded code.  A trivial request in main
> suffices, and that change already landed in 3.108.

Sorry, I just faithfully moved those changes from timeless' original patch. I've left them out of this version.

> XPC_NW_NewResolve should use a single request, not two in a row in the case
> where the ::JS_DefineUCProperty call is reached.

Ok.

Comment 42

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

Attachment: [NOT FOR CHECKIN (-w)] Request Model with JSAutoRequest (Trunk) v1.4

Comment 43

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

(In reply to comment #13)
> Created an attachment (id=183611) [edit]
> updated jsapi.h patch

Brendan, I tried applying this to the 1.8 branch and got some compilation errors around the private members... Then I checked the trunk and both of the private members are #if 0'ed out... Any particular reason for that?

Comment 44

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

(In reply to comment #43)

Of course when I said members I really meant operators.

I need more coffee.

Comment 45

[Brendan Eich [:brendan]]

(In reply to comment #44)
> (In reply to comment #43)
> 
> Of course when I said members I really meant operators.

CPP_THROW_NEW is not macro-ized for SpiderMonkey builds, or so I worried.  True?

/be

Comment 46

[Brendan Eich [:brendan]]

Comment on attachment 219035 [details] [diff] [review]
Request Model with JSAutoRequest (Trunk) v1.4

I say ask mrbkap for help landing, and let's see how this affects tinderbox Ts, Txul, and Tp metrics.

/be

Comment 47

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

(In reply to comment #45)
> CPP_THROW_NEW is not macro-ized for SpiderMonkey builds, or so I worried. 

I filed bug 335414 to fix the '#ifdef 0' block around those private operators and have CPP_THROW_NEW defined.

Comment 48

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

Attachment: updated jsapi.h patch (1.8 branch)

Here's the patch for the 1.8 branch to add JSAutoRequest to jsapi.h.

Comment 49

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

Attachment: Request Model with JSAutoRequest (1. branch) v1.0

Here's the patch for the 1.8 branch.

Comment 50

[timeless]

(In reply to comment #40)
> Please don't patch js.c, it is single-threaded code.  A trivial request in main
> suffices, and that change already landed in 3.108.

brendan: erm. please see comment 36. and remember that while js.c is normally not multithreaded, i don't think anything prevents a tester from enabling threading and adding in their own thread objects which is precisely what xpcshell does.

Comment 51

[timeless]

specifically, perlconnect and liveconnect enabled jsshells probably are able to do thread stuff, although i don't have time at the moment to create those testcases.

Comment 52

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

/me pokes mrbkap ;-)

Comment 53

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

After this lands I'll file a followup bug to make sure everything works with JS_PARANOID_REQUEST defined.

Comment 54

[timeless]

OK, bz tested http://webwizardry.net/~timeless/176182 (minus the xmlhttprequest stuff, my merge didn't include the merging step of moving them to their new location and updating again).

He reported: no significant difference in Tdhtml or Ts.  Txul might have
regressed by about 1%, but it could also be noise....

for reference the patch was generated using the tools on http://viper.haque.net/~timeless/patchtools/ and following the instructions at the bottom of that page (be sure to View>Use Style>Verbose Text).

I might as well catalog it here since it seems I like sticking detailed tutorials in strange places. First you need a CVS/ directory that you can use. 

this only needs to be done once:
(cd /tmp; mkdir bonsai; mkdir bonsai/CVS; (cd bonsai/CVS; wget http://viper.haque.net/~timeless/patchtools/bonsai/CVS/cvs.zip; unzip cvs; rm cvs.zip)

the actual merge process (bug 176182 is this bug and becomes the sandbox. attachment 219035 [details] [diff] [review] is the patch we're merging, which I call 'a' because it's short and tends to be used a lot by the scripts in my toolbox)
cd /tmp
mkdir 176182; cp -R bonsai/CVS/ 176182; cd 176182
wget -Oa 'http://bugzilla.mozilla.org/attachment.cgi?id=219035&action=view'
mergePcvs.pl a ; patch -d `pdir.pl a` -p0 < a ; cvs up -A

what you do from here kinda depends. if you have conflicts, obviously you need to resolve them. in this case, there are some files which CVS indicates are dead. that means a trip to bonsai to find out where they went. you have two choices:
1. pray that the death blow included a reference to the new location (use cvslog on the file you have): http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp
2. pray that the file was copied to a new place w/ the same name: http://bonsai.mozilla.org/cvsguess.cgi?file=nsXMLHttpRequest.cpp

#2 is the approach I like to take, and it works here, it says the new location is:
mozilla/content/base/src/nsXMLHttpRequest.cpp

OK, so, what to do with this info?
well, we because it was a cvs copy, the version of the file we have:

cvs status mozilla/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp|grep "Working"

(1.147) in the current location does exist in the new location, so we can check it out:
cvs co -r 1.147 mozilla/content/base/src/nsXMLHttpRequest.cpp
and then overwrite it with our modified version from the same version
cp mozilla/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp mozilla/content/base/src/nsXMLHttpRequest.cpp
cvs up -A mozilla/content/base/src/nsXMLHttpRequest.cpp

If it turns out that the file moved again, then you used the wrong cvsguess :), but you could repeat the process for this file, and you can repeat this process for any other moved files.

once you're done you can make a new patch (but you should probably build first and see that it works).

cvs diff -up > mergedpatch
cvs commit `cvsu --types=MGAR --find`

Anyway, this is merging as I do it. One important part from my perspective is it means that a cvs diff -U0 is perfectly fine, since I can use mergePcvs to get the same version and then DIFFCMD=-pU1000 cvsdo diff to read it as I like it. But more importantly, since we're using cvs to do the merges, it generally does a much better job than hand applying patches.

bent is free to try this technique to merge the patch or hopefully he has a live version in his tree.

of course, I'd appreciate it if my review comments would override brendan's and he'd incorporate them into the next patch.

Comment 55

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

Attachment: Request Model with JSAutoRequest (Trunk) v1.5

(In reply to comment #53)
> After this lands I'll file a followup bug to make sure everything works with
> JS_PARANOID_REQUEST defined.

Actually, I did that already with this patch. Everything seems to work fine.

Let's get this one in before it rots!

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

I checked the last patch into the trunk. balsa went orange with what seems like an unrelated problem, and nye went orange on the same test, probably for the same reason. Marking this bug as FIXED.

Comment 57

[Boris Zbarsky [:bzbarsky]]

So Tdhtml on luna regressed by about 1.5% in the checkin range that includes this bug:

http://build-graphs.mozilla.org/graph/query.cgi?testname=dhtml&tbox=luna.mozilla.org&autoscale=1&days=7&avg=1&showpoint=2006:06:13:07:40:50,1499
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-06-12+14%3A38%3A20&maxdate=2006-06-12+15%3A52%3A19&cvsroot=%2Fcvsroot

Do we know why?  Can we do something about it?

Comment 58

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

So is this something that we could get on the branch? I can whip out a branch patch if it seems likely that it won't rot - maybe after 1.8.1b1 ships?

Comment 59

[Mike Schroepfer]

Can you give us a better sense of why we'd want this on the 1.8 branch.  We'd need a strong reason to take it given the Tdhtml regression noted in comment 57 (or a fix that doesn't include the perf regression).

Comment 60

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

(In reply to comment #59)
> Can you give us a better sense of why we'd want this on the 1.8 branch.  We'd
> need a strong reason to take it given the Tdhtml regression noted in comment 57
> (or a fix that doesn't include the perf regression).

About the perf regression: The numbers pre-checkin on luna were about 1475-1490. Post-checkin they jumped to ~1505. That seems to have stayed stable. I'm not sure exactly how bad that is or how it could be fixed.

The benefit of this patch is that embedding and XULRunner apps can use multiple threads without immediately crashing in the garbage collector. Right now any time a XR app touches the DOM from more than one thread a GC race condition can occur which leads to frequent crashes. For songbird the problem was a complete blocker on linux as we couldn't even start the app without crashing. It was also a prevalent problem on mac. Windows seemed largely unaffected but many of our mysterious/unpredictable crashes disappeared after applying this patch so I think our Windows version was just getting lucky. In any case this patch is essential if your XR app is multithreaded.

I believe that XR from 1.8.1 is going to be pushed as a stable platform (am I making that up?) so I think taking this patch should be high priority.

Oh, and as far as I know this patch caused no regressions (beyond the perf hit).

Comment 61

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

Looks like there was an accidental change to jsd_val.c in this fix. Check out bug 342573.

Comment 62

[Brendan Eich [:brendan]]

(In reply to comment #60)
> About the perf regression: The numbers pre-checkin on luna were about
> 1475-1490. Post-checkin they jumped to ~1505. That seems to have stayed stable.
> I'm not sure exactly how bad that is or how it could be fixed.

First step would be to profile the tests on that box or a similar box.  bz, dbaron, graydon, and vlad may have profiler suggestions.

/be

Comment 63

[Brendan Eich [:brendan]]

(In reply to comment #61)
> Looks like there was an accidental change to jsd_val.c in this fix. Check out
> bug 342573.

Preparing a branch patch might be worth doing now, to clear the way.

/be

Comment 64

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

Attachment: Request Model with JSAutoRequest (1.8 Branch) v1.1

(In reply to comment #63)
> Preparing a branch patch might be worth doing now, to clear the way.

Here we go. This incorporates the JSAutoRequest definition in jsapi.h, the addition of JSAutoRequest objects throughout the codebase, and the cleanup of jsd_val.c. In short this is everything needed for the branch.

Comment 65

[Boris Zbarsky [:bzbarsky]]

On linux, both jprof and sysprof are good, with slightly different uses -- jprof is better if you want to profile specific tasks, since you can start and stop it in JS, whereas sysprof is better if you want to include work done outside Mozilla (eg in X).

For what it's worth, I ran a quick profile on http://www.speich.net/computer/3d.php and it looks like so:

Total hit count: 288294
  3419 under JS_EndRequest
  1549 under JS_BeginRequest

That's a total of about 1-2%.  The main callers of JS_EndRequest are:

               2978 JS_SuspendRequest
                108 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)
                 98 nsXPConnect::WrapNative(JSContext*, JSObject*, nsISupports*, nsID const&, nsIXPConnectJSObjectHolder**)
                 77 nsGenericArraySH::NewResolve(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, unsigned int, JSObject**, int*)
                 65 nsArraySH::GetProperty(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, long*, int*)
                 48 nsDOMClassInfo::GetArrayIndexFromId(JSContext*, long, int*)

with the JS_SuspendRequest calls coming from:

               3548 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)
                160 XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned int, long*, long*)

the JS_BeginRequest are coming from:

                945 JS_ResumeRequest
                214 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)
                113 nsGenericArraySH::NewResolve(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, unsigned int, JSObject**, int*)
                113 nsXPConnect::WrapNative(JSContext*, JSObject*, nsISupports*, nsID const&, nsIXPConnectJSObjectHolder**)
                 88 nsArraySH::GetProperty(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, long*, int*)
                 41 nsDOMClassInfo::GetArrayIndexFromId(JSContext*, long, int*)

and JS_ResumeRequest comes from:

               1182 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)
                186 XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned int, long*, long*)

(no surprises there).

As to the costs, JS_BeginRequest breaks down as:

       858     1549 JS_BeginRequest
                233 PR_Lock
                157 PR_Unlock
                 74 pthread_mutex_lock
                 65 pthread_mutex_unlock
                 59 __i686.get_pc_thunk.bx
                 41 pthread_self
                 33 pthread_equal

That's about 50+% of the time in the function itself, the rest in lock/unlock stuff.

JS_EndRequest breaks down as:

       634     3419 JS_EndRequest
               1418 PR_Unlock
                645 PR_NotifyCondVar
                337 PR_Lock
                106 pt_PostNotifyToCvar
                 90 pthread_mutex_lock
                 60 pthread_self
                 40 pt_PostNotifies

So again, mostly locking costs.

The problem with CallMethod, of course, is that we have to suspend the request while we call into the C++ code... 

Comment 66

[Darin Fisher]

Not going to block 1.8.1 for this, but we'll happily consider a patch for the 1.8 branch.

Comment 67

[Darin Fisher]

Comment on attachment 226929 [details] [diff] [review]
Request Model with JSAutoRequest (1.8 Branch) v1.1

re-request approval1.8.1 when you have review+.  thanks!

Comment 68

[Brendan Eich [:brendan]]

Comment on attachment 226929 [details] [diff] [review]
Request Model with JSAutoRequest (1.8 Branch) v1.1

Still wanted?  Up to date?

/be

Comment 69

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

Comment on attachment 226929 [details] [diff] [review]
Request Model with JSAutoRequest (1.8 Branch) v1.1

Thanks, brendan, but we're in the process of moving to 1.9 atm... We've been manually applying the patch to 1.8 up to this point.