916988 - Various analysis updates

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Laundry list of analysis changes

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: AutoCompartment constructor leads to too many false positives

If an exception is pending when we enter an AutoCompartment scope, it could get wrapped, which could trigger GC. But treating that as a cangc function introduces false positives in the common places where no exception will ever be set. So for now, I'm just annotating the AutoCompartment constructor overload that is causing issues.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Treat mozilla::dom::Rooted* as a rooted type

bz is now using things like mozilla::dom::RootedTypedArray..., which should be considered Rooted. They inherit from AutoGCRooter (or maybe CustomAutoRooter; doesn't matter which) and so should theoretically be automatically handled. But the analysis currently can't distinguish inheritance from just having an initial data member of the superclass, and the current processing is order-dependent, and it's a heuristic snarl to begin with (because it's unclear what an AutoRooter is rooting.) RootedTypedArray uses multiple inheritance, which makes it even harder. So punt for now and just key off of the type name the same way we are doing it in spidermonkey.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Minor variable rename in rooting analysis,

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: NS_IsMainThread does not GC

NS_IsMainThread causes nearly 200 false positive hazards. NS_IsMainThread is considered a GC function because it calls nsIThreadManager.GetIsMainThread, but annotating just that reveals that it's also a gc function because:

GC Function: uint8 NS_IsMainThread()
    nsCOMPtr<T>::nsCOMPtr(nsGetServiceByContractID) [with T = nsIThreadManager]
    void nsCOMPtr<T>::assign_from_gs_contractid(nsGetServiceByContractID, const nsIID&) [with T = nsIThreadManager; nsIID = nsID]
    uint32 nsGetServiceByContractID::operator(nsID*, void**)(const nsIID&, void**) const
    uint32 CallGetService(int8*, nsID*, void**)
    FieldCall: nsIServiceManager.GetServiceByContractID

Those sound kind of scary to whitelist, so it seems simplest to just annotate NS_IsMainThread.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 805564 [details] [diff] [review]
AutoCompartment constructor leads to too many false positives

Review of attachment 805564 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 805569 [details] [diff] [review]
Treat mozilla::dom::Rooted* as a rooted type

Review of attachment 805569 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 805572 [details] [diff] [review]
NS_IsMainThread does not GC

Review of attachment 805572 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. Hopefully, one day we'll have C++11 [[annotation]] support and can at least put all these in the source.

Comment 8

[Steve Fink [:sfink] [:s:]]

Whoops, just noticed that I never checked these in. It was hard to tell because the automation box already has these changes, and I've been pushing the try hazards builds with these patches applied. But I just looked at my first successful end-to-end JS shell hazards build on try -- https://tbpl.mozilla.org/php/getParsedLog.php?id=28172991&tree=Try&full=1 producing http://ftp.mozilla.org//pub/mozilla.org/firefox/try-builds/[e-mail]-962b0d2dec87186a0220a7e552855802a20693e8/try-linux64-sh-haz -- and it was complaining about NoGC functions again!

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b075021285f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e24a63ca36
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec5c70efd2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4df586670d2a

Btw, if you push to try with -p linux64-sh-haz, you'll now get a build that uploads hazards.txt and gcFunctions.txt to the ftp server. The full-browser version still isn't quite working.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b075021285f
https://hg.mozilla.org/mozilla-central/rev/e6e24a63ca36
https://hg.mozilla.org/mozilla-central/rev/1ec5c70efd2e
https://hg.mozilla.org/mozilla-central/rev/4df586670d2a