We expose a small set of hooks to servo for the worker threads to traverse the DOM and (sometimes) set computed values on the style structs. I've been manually auditing them to make sure they're safe to on arbitrary threads, but that approach is obviously fragile. One problem I ran into recently was bug 1292662. In order to properly handle all the edge cases with NAC, XBL, and Shadow DOM, we need to call into some medium-scale Gecko machinery for traversing children and parents (AllChildIterator and GetFlattenedTreeParent respectively). These seem _mostly_ ok, but the call space balloons pretty fast. I'd like to find some sort of static or dynamic analysis to protect against these cases. The general problem of finding races is certainly a hard one, but our constraints here are simpler. Namely, the main thread is blocked, and we have an arbitrary number of worker threads examining the DOM. As long as those worker threads don't mutate anything on the heap, there should be no races. Michael, how tractable would it be to use static analysis to forbid non-whitelisted heap mutations within specific subtrees of the call graph?

Currently our clang static analysis infrastructure doesn't support cross compilation-unit analysis, so any analysis we perform with the existing infrastructure would have to work within a single compilation unit. I imagine that the call graphs for these functions spans compilation units. To get around that, annotations could be added to the functions. For example, we could have a MOZ_NO_HEAP_MUTATIONS annotation on a function to signify that that function doesn't perform heap mutations, perhaps combined with a MOZ_MUTATES annotation on arguments to signify that if those arguments are on the heap then it would be a problem? We could then make the static analysis check that these annotations are correct. This would probably be a non-insignificant amount of work, however, to annotate all of those functions. We could also look into building infrastructure for global analysis (with presumably a data collection phase, and then an error reporting phase), which could also be a lot of work but might enable more analysis in the future.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #0) > Michael, how tractable would it be to use static analysis to forbid > non-whitelisted heap mutations within specific subtrees of the call graph? This is basically what the rooting analysis does, isn't it?

(In reply to Nathan Froyd [:froydnj] from comment #2) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #0) > > Michael, how tractable would it be to use static analysis to forbid > > non-whitelisted heap mutations within specific subtrees of the call graph? > > This is basically what the rooting analysis does, isn't it? Indeed. Steve, how tractable would it be to expand the rooting analysis to cover this?

I certainly have the global callgraph in hand, so that part is done. But to answer the question, I'd need to know exactly what you mean by heap mutations to figure out how feasible it would be to detect them. (It wouldn't be too hard to pattern-match memory writes, but there's no straightforward way to distinguish heap vs stack. Unless there's some static type info?) If the clang-based analyses have per-compilation unit callgraph info, and can detect these heap writes, it'd probably be easier to stitch together the global callgraph and do it there. My analysis infrastructure is very good at some things and very bad at others. I have the global callgraph with pretty decent virtual function call handling. At a finer granularity I can identify regions within an RAII scope, for example, and I also have the full intra-function CFGs so I can compute dominators within functions. I also have pretty good type information. But I don't have dataflow, and I see things at a pretty low level (eg lots of things produce temporaries, and I only see uses of or writes to the temporary and can't track it back to the statement or whatever.) I'm not who to ni? here. To proceed with my analysis infrastructure, I'd have to have detailed info on the heap writes of interest. To proceed with the clang stuff, Michael would have to weigh in on the state of the callgraph stuff.

This isn't quite like the rooting analysis, since as Steve said, the job of that analysis is simply "can this function be called indirectly" which is quite simple to determine statically. Detecting whether a function mutates some piece of data that lives on the heap is impossible, since even if you annotated all of the variables in your program to indicate whether they hold data on the heap, pointers can change what they point to dynamically so it's impossible to reason about their pointee data location statically. Bobby, can you think of another way to express the things you would like to detect? If not, I'm afraid this is INVALID.

<bholley> ehsan: yt? <@ehsan> bholley: yes <bholley> ehsan: so, I'm not convinced the situation is as intractable as you suggest <@ehsan> ok <bholley> ehsan: in particular, I think it's fine to have a relatively restrictive set of rules <bholley> ehsan: and do explicit annotations, if need be <@ehsan> bholley: I have nothing against that <bholley> ehsan: for example, suppose the only writes we allowed were assigning to locals and to arguments <@ehsan> bholley: as long as it's _possible_ to annotate <@ehsan> arguments as in assigning to the pointee of pointer args? <bholley> ehsan: the case I'm thinking about is the nsTArray in AppendAnonymousContentTo <bholley> ehsan: we could also just whitelist that somehow <bholley> it's a pretty unusual API <@ehsan> bholley: ok, that would work <@ehsan> bholley: (note that the other missing half is global program analysis) <@ehsan> so it's like we can implement that check today <@ehsan> but this new formulation is _possible_ to check statically <bholley> ehsan: but can't we just use explicit annotations to whitelist the callgraph? <bholley> ehsan: i.e. you can only call other whitelisted functions <bholley> ehsan: or do virtual calls screw that up <@ehsan> bholley: that will soon get out of hand <@ehsan> that's one problem <@ehsan> the other is that you'd need to over annotate <@ehsan> think of things such as utility functions <@ehsan> mfbt goo, smart pointers etc <bholley> fair <@ehsan> and you may end up with functions that need to be annotated and not annotated at the same time <@ehsan> or even worse <@ehsan> you'd need the annotation to apply only to some template instantiations <@ehsan> which make the annotations inexpressible in C++ <@ehsan> and so on <@ehsan> bholley: but that being said, if we find something tractible to annotate that should not be too hard <@ehsan> the whole callgraph is unfortunately not it <@ehsan> cc mystor fyi ^ <bholley> ehsan: so what do you suggest? <@ehsan> bholley: can you please write a few paragraphs describing the problems on the bug? <@ehsan> bholley: I feel like I need to learn more about what you need before I can suggest something useful :/ <bholley> ehsan: which problems? <bholley> ehsan: IIUC you understand the motivation and use-cse <bholley> *case <@ehsan> bholley: the race issues you're trying to prevent <mystor> I assume you're talking about the validating stuff? <bholley> mystor: yes <@ehsan> bholley: honestly I'm still a bit in the dark about why mutations themselves are the issue <@ehsan> bholley: (so far I understand the obvious part about main thread only functions being a no no) <bholley> ehsan: they're not inherently an issue, but we don't need them and gain a lot of safety by forbidding them <bholley> ehsan: and in particular <mystor> I feel like if we wanted to do a good job of it and not annotate the universe, we would have to set up some infrastructure for global clang static analysis, even if just a simple one, such as a sqlite database which the clang plugin can write to, and then a phase which runs after building which checks that database. <bholley> ehsan: the servo traversal code maintains thread safety by partitioning the DOM and traversing different nodes on different threads <bholley> ehsan: but if C++ goes and mutates something in the heap, that could have UB interactions with the other worker threads if they happen to read that data <bholley> ehsan: conceptually, we're doing a read-only traversal <bholley> ehsan: which makes it safe <bholley> ehsan: and I would like to enforce that concept and make it more of a guarantee <@ehsan> bholley: so if we wanted to ban anything other than accesses to function locals and arguments (pretending that callers are expected to only pass addresses to locals to callees) <@ehsan> bholley: would that be sufficient? <bholley> ehsan: certainly miles of improvement over the status quo <@ehsan> mystor: yeah whole program analysis is definitely a prerequisite, we can't do this without <@ehsan> bholley: that should be doable <@ehsan> bholley: with almost no annotations <@ehsan> bholley: besides annotating a function as MOZ_STYLO_TRAVERSAL or some such <bholley> ehsan: that's great <@ehsan> (that would be the "entry" function) <bholley> ehsan: the functions I'm trying to make safe are things like http://searchfox.org/mozilla-central/source/dom/base/FragmentOrElement.cpp#155 <bholley> ehsan: which we need to call into if we're traversing anonymous nodes <bholley> ehsan: in order to get the binding-aware parent <@ehsan> bholley: fwiw there are also other use cases for whole program analysis for our future plans for DOM as well, so that's something we have to do at some point soon <bholley> ehsan: and it just does all this stuff and is hard to audit by hand <bholley> ehsan: :-) <@ehsan> bholley: whoa wait <@ehsan> bholley: that function doesn't adhere to the contract above right? <@ehsan> since it uses nsTArray <@ehsan> which needs to assign to its members <bholley> ehsan: but it never mutates the nsTArray, no? <@ehsan> bholley: true <@ehsan> nm :) <@ehsan> bholley: FWIW we can also have a "shut up about this part of this function" annotation if need be <bholley> ehsan: yes, that's wise <bholley> ehsan: opt-outs are always good <@ehsan> yeah <@ehsan> bholley: sorry gotta run to a meeting now, but looks like we got to something good! <bholley> ehsan: ok, what's the next step? <@ehsan> bholley: I need to figure out how to make the whole program analysis happen <@ehsan> very high on my todo list <bholley> ehsan: ok sounds good - I'll NI <@ehsan> ty

It's unclear whether we're going to do whole-program analysis for now. I'll post here if the plans change. Clearing the needinfo for now.

(In reply to :Ehsan Akhgari from comment #7) > It's unclear whether we're going to do whole-program analysis for now. Did I miss a conversation somewhere? I understand that the requirements of the DOM work may have changed, but we still want this for stylo...

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8) > (In reply to :Ehsan Akhgari from comment #7) > > It's unclear whether we're going to do whole-program analysis for now. > > Did I miss a conversation somewhere? I understand that the requirements of > the DOM work may have changed, but we still want this for stylo... There wasn't much conversation, we just don't know if it's going to be needed for pre-emption in DOM yet. (Not saying that it won't be needed, just saying that it's not obvious that it is.)

Just discussed this on IRC: > <ehsan> bholley: that's not necessarily true. I was going to suggest maybe before trying something super hard, we can try to see if sixgill is enough for your needs (it should be) > <ehsan> bholley: and neither mystor nor I are particularly familiar with sixgill, so anyone can do that work > <ehsan> which means we can go back to doing the work we have already signed up for :) > <bholley> ehsan: I thought the conclusion of our discussion was that sixgill was hard we wanted to do it with clang static analysis? (comment 4) > <ehsan> bholley: we may not need something too generic though > <ehsan> sixgill knows about to reason about the callgraph for GC calls > <bholley> ehsan: can you suggest who would be the person to figure out exactly what we need? I'm sorry to bug you, but the alternative is just that the project management people will start bugging you about it in a week or two > <ehsan> bholley: we have two people who know about sixgill, bhackett who wrote it, and sfink who uses it > <mccr8> I poked a little at hacking up sixgill to do something else and with sfink's guidance it wasn't too hard to get it at least sort of working. > <ehsan> bholley: but since both of those are busy people and there's going to be some learning in this project, I'd say any platform engineer who's ready to learn some new stuff? > <ehsan> bholley: we can probably start by looking at the existing rooting hazard analysis as an example, so it may not even be too much learning > <bholley> ehsan: my concern is that the last time we asked sfink he didn't think it was the right approach > <ehsan> bholley: we have exactly two alternatives here: > <bholley> ehsan: you appear to have a different opinion now, but I'd still like to at least have consensus between the two of you before we send somebody charging at it > <ehsan> 1. invent our own thing. this is what I meant to do before when we had at least two real use cases, but now we have only 1 (for now at least) > <ehsan> 2. reuse an existing solution that can generate callgraphs > <ehsan> bholley: I'm not familiar enough with sixgill to say for sure that 2 won't work (and if sfink says so we should find out why) > <ehsan> but I'm familiar enough with #1 to say that it will take a significant amount of effort > <ehsan> (even the practicality isn't 100% clear) > <ehsan> since now we only have one real consumer for this type of analysis, I suggest we should start looking into 2 > <ehsan> bholley: makes sense? > <ehsan> (I *still* want to have #1, but I have wanted it for years now, and it has never been high enough on the priority list to be worth working on :/ ) > <ehsan> bholley: to rephrase, I'm trying to give you a more practical than pie in the sky solution :) > <bholley> ehsan: can you estimate the effort level of #1, as well as the relative value over #2 such that we can compare with sfink's estimate for #2? > <ehsan> bholley: estimating it is hard since I haven't even started to think about the details yet, but the order of complexity is probably a few human-quarters > <mystor> I think a lot of how hard #1 is depends on how good of a job we try to do of it > <ehsan> yeah > <mystor> If we try to make a generic callgraph understanding global analysis solution it will be hard. If we want to make a one-off works-on-my-machine solution, that isn't hard > <mystor> I'm guessing the minimum requirement is something in the middle > <ehsan> bholley: I'd say #1 isn't worth doing at all unless if we want to build something for generic analyses, with the hopes of future analyses paying for its up front cost > <ehsan> mystor: a one off solution won't work for bholley's needs, since presumably we want to maintain stylo long term > <bholley> unless "my machine" is a TC image > <mystor> Hence why I said I expected we wanted something in the middle > <ehsan> bholley: that's the thing though! it's so easy to do this in a way that would take days to run each time > <ehsan> bholley: doing this efficiently is really hard > <bholley> ok, I will NI sfink and see what he thinks - thanks! > <ehsan> bholley: so, reading https://bugzilla.mozilla.org/show_bug.cgi?id=1294915#c4, are these the concerns you mentioned? > <bholley> ehsan: yes > <ehsan> bholley: can you please refresh my memory about the stack vs heap question? > <ehsan> do we still need that? > ehsan sees in comment 6 that we agreed banning accesses to things other than function locals and arguments > <ehsan> ok so that's not an issue > <ehsan> bholley: and this one doesn't need dataflow, so it seems like both of the points sfink was worried about aren't an issue in this case? > <bholley> ehsan: I don't really know enough about static analysis to answer that, but it sounds like you're answering it for me :-) > <ehsan> bholley: I _think_ dataflow isn't needed in this case > <ehsan> bholley: would be worth double checking with sfink > <bholley> ehsan: ok, I'll NI him > <ehsan> bholley: thanks! > <bholley> ehsan: thanks for the help :-) So it sounds like we should ask sfink again what he thinks about doing this with sixgill.

I can't say I'm totally clear on the requirements here, but let me write out some thoughts. As I said, the global nature of the problem is not a problem for the hazard analysis. Sixgill works locally (in the way we're using it), but records everything into a database so we can (and do) build a global callgraph, and we have random access to all functions' control flow graphs. So the difficult is in recognizing the problematic patterns. Sixgill converts GCC's full control flow graph into a simplified "intermediate language" graph involving only 3 types of edges: Assume, Assign, and Call[1], and they operate on expressions (eg dereference and write). Temporaries are created as needed, so we would need to do a data flow analysis propagating a could-be-heap property throughout the program, and determine whether anything dereferences and writes to could-be-heap pointers. I don't think it would be enough to just look for direct writes into parameters and locals, since many operations will create temporaries and multiple values can flow into the same temporary. So it seems like there are two possible approaches: 1. Assume that parameters always refer only to things on the stack. Then this would be a purely local analysis that forbids any could-be-heap pointers from either being written to or reaching any Call. You wouldn't need a global analysis at all. But this seems pretty restricting. 2. Allow locals to be written to and passed into Calls. Globally trace through and figure out which parameters can be written to, and disallow could-be-heap pointers from being passed as arguments to those parameters. Assume any unknown function (eg from glibc) writes into all of its parameters, but have explicit whitelists. (This is for things like memset. Maybe the default could go the other way, but this way is safer.) Do these match what you were thinking of? I'm really not sure (a) how doable this is with the sixgill data structures, (b) how much work it would be, or (c) whether it would have too many false positives to be useful. I think I'll defer to bhackett for an opinion on the feasibility, since he has done whole program analysis like this before. (Maybe using the unused capabilities of sixgill would work here?) [1] This is simplified; there are a few more types of edges, but they're not very relevant here.

sixgill is certainly powerful enough to do this sort of thing. I've found what helps the most to build a reliable, maintainable analysis is KISS (keep it simple, stupid), and to that end a dataflow analysis should be avoided (they are monstrously complex, especially for C/C++). I think it's possible to do a good job here without doing any real dataflow. One basic design: 1. Use the sixgill callgraph to find the portions of the callgraph where heap mutations are disallowed (this is I guess all reachable functions from a fixed list of roots). 2. Within each function where heap mutations are disallowed, look at all indirect writes (those which involve a dereference and could be on a heap location). 3. For each indirect write, report an error if the write is not to a field of a MOZ_STACK_CLASS structure. There are a few additional wrinkles for implementing this. 1. The sixgill callgraph handles virtual calls, but not function pointer calls (figuring out their targets requires dataflow analysis). Any function pointer calls in places where heap mutations are disallowed should either report an error or be whitelisted. (One place this could come up is if any of the involved functions are in a PLDHashTable or something, which uses function pointers.) 2. Functions which we don't have the implementations for (e.g. from glibc, as Steve mentioned) won't appear to have any indirect writes. Calls to these functions from places where heap mutations are disallowed should either report an error or be whitelisted. 3. Writes through buffers or other pointers to primitive types (e.g. 'int x; int* y = &x; *y = 3;') will always be treated as heap writes here. Hopefully there won't be too much of this to deal with, otherwise it might be necessary to make the analysis more complicated or use more whitelist annotations. 4. I don't know offhand if MOZ_STACK_CLASS is accessible through the sixgill machinery. If not it could be done, but classes could also be manually whitelisted for the purposes of building a prototype for the analysis.

(In reply to Brian Hackett (:bhackett) from comment #12) > 3. Writes through buffers or other pointers to primitive types (e.g. 'int x; > int* y = &x; *y = 3;') will always be treated as heap writes here. > Hopefully there won't be too much of this to deal with, otherwise it might > be necessary to make the analysis more complicated or use more whitelist > annotations. I imagine that this will be too restrictive because of things like: bool result; nsresult rv = docShell->GetIsFoo(&result); Which will almost certainly appear somewhere in the callgraph and be reported as a write to heap data structures. I imagine that pointers to primitive types which are passed in parameters could probably be safely whitelisted.

(In reply to Steve Fink [:sfink] [:s:] from comment #11) > 1. Assume that parameters always refer only to things on the stack. Then > this would be a purely local analysis that forbids any could-be-heap > pointers from either being written to or reaching any Call. You wouldn't > need a global analysis at all. But this seems pretty restricting. This wouldn't work, because we could never call methods on DOM objects (since that would be passing a could-be-heap pointer to Call as |this|). (In reply to Michael Layzell [:mystor] from comment #13) > (In reply to Brian Hackett (:bhackett) from comment #12) > > 3. Writes through buffers or other pointers to primitive types (e.g. 'int x; > > int* y = &x; *y = 3;') will always be treated as heap writes here. > > Hopefully there won't be too much of this to deal with, otherwise it might > > be necessary to make the analysis more complicated or use more whitelist > > annotations. > > I imagine that this will be too restrictive because of things like: > > bool result; > nsresult rv = docShell->GetIsFoo(&result); > > Which will almost certainly appear somewhere in the callgraph and be > reported as a write to heap data structures. I imagine that pointers to > primitive types which are passed in parameters could probably be safely > whitelisted. Indeed. I wonder if we could even enforce that any pointers to primitive types passed in the callgraph are either arguments or locals? Brian's proposal seems like it's worth a try. One wrinkle which we'd need to handle somehow is the nsTArray out-params in AppendAnonymousContentTo. It's a rare case, so probably easiest to just whitelist that argument to that function somehow. Brian, do you by any chance have cycles in the next month or so to prototype this analysis? A pretty good testcase would be Gecko_GetNextStyleChild [1], which calls into StyleChildrenIterator::GetNextChild [2]. That function probably has the most complex and interesting callgraph, so if we can make this work relatively non-intrusively for that, it's pretty likely to do the job. [1] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/layout/style/ServoBindings.cpp#154 [2] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/dom/base/ChildIterator.cpp#591

Sure, I can take a look within the next couple of weeks.

(In reply to Brian Hackett (:bhackett) from comment #15) > Sure, I can take a look within the next couple of weeks. Awesome - thank you so much! This is super helpful for Quantum. :-)

(Also note that StyleChildrenIterator itself is not MOZ_STACK_CLASS, since we need to heap-allocate it for usage by servo. So we'll need some slightly-broader annotation, which MOZ_STACK_CLASS can imply).

Analysis WIP. This starts from a root function (Gecko_GetNextStyleChild is baked in for now) and scans through it and its callees until it finds a call or a write whose behavior might not be threadsafe: a call target's body is missing or the targets are unknown, or a write might be to a heap location. When it finds one it prints out info and then stops. This can then be fixed with an annotation and the analysis rerun. Running the analysis takes several seconds to find an error, due to needing to load information about all class/struct/union types at startup (necessary for now to resolve virtual calls; this could be improved). An alternative here would be to print out N errors before stopping. I started this out by printing out all errors, but it isn't practical right now because the callgraph search is finding functions where state is lazily constructed or where QIs are performed, which tend to pull in tons of callees. I haven't looked at these in enough detail to determine whether they are legitimate. Anyways, after a little bit of annotation (which are at all at the top of this analysis file) I found what looked to my layman's eye like the first potential issue. This is the printed message: Error: Field write mozilla::FramePropertyTable.mLastFrame Location: void* mozilla::FramePropertyTable::GetInternal(nsIFrame*, mozilla::FramePropertyDescriptorUntyped*, uint8*) @ layout/base/FramePropertyTable.cpp:73 Stack Trace: mozilla::FramePropertyTable::PropertyType<T> mozilla::FramePropertyTable::Get(const nsIFrame*, mozilla::FramePropertyTable::Descriptor<T>, bool*) [with T = nsAbsoluteContainingBlock; mozilla::FramePropertyTable::PropertyType<T> = nsAbsoluteContainingBlock*; mozilla::FramePropertyTable::Descriptor<T> = const mozilla::FramePropertyDescriptor<nsAbsoluteContainingBlock>*] @ layout/base/FramePropertyTable.h:215 mozilla::FrameProperties::PropertyType<T> mozilla::FrameProperties::Get(mozilla::FrameProperties::Descriptor<T>, bool*) const [with T = nsAbsoluteContainingBlock; mozilla::FrameProperties::PropertyType<T> = nsAbsoluteContainingBlock*; mozilla::FrameProperties::Descriptor<T> = const mozilla::FramePropertyDescriptor<nsAbsoluteContainingBlock>*] @ layout/base/FramePropertyTable.h:421 nsAbsoluteContainingBlock* nsIFrame::GetAbsoluteContainingBlock() const @ layout/generic/nsFrame.cpp:257 nsFrameList* nsFrame::GetChildList(uint32) const @ layout/generic/nsFrame.cpp:1493 nsFrameList* nsContainerFrame::GetChildList(uint32) const @ layout/generic/nsContainerFrame.cpp:278 nsFrameList* nsBlockFrame::GetChildList(uint32) const @ layout/generic/nsBlockFrame.cpp:585 nsFrameList* nsComboboxControlFrame::GetChildList(uint32) const @ layout/forms/nsComboboxControlFrame.cpp:1427 nsIFrame* nsLayoutUtils::GetAfterFrameForContent(nsIFrame*, nsIContent*) @ layout/base/nsLayoutUtils.cpp:1583 nsIFrame* nsLayoutUtils::GetAfterFrame(nsIFrame*) @ layout/base/nsLayoutUtils.cpp:1595 nsIContent* mozilla::dom::AllChildrenIterator::GetNextChild() @ dom/base/ChildIterator.cpp:451 nsIContent* mozilla::dom::StyleChildrenIterator::GetNextChild() @ dom/base/ChildIterator.cpp:593 Gecko_GetNextStyleChild @ layout/style/ServoBindings.cpp:159 Bobby, is this the sort of thing you had in mind? I can suppress this one with an annotation and keep cranking away at resolving errors, or if anyone wants to try this themselves they need to build sixgill and will need this file and a couple others I'll attach in a minute, plus the sixgill databases for the tree they're analyzing. If necessary I can upload my databases from a recent m-i somewhere.

Utility file for traversing the callgraph. This is code pulled out from js/src/devtools/rootAnalysis/computeCallgraph.js. Running the heap write analysis also needs utility.js and annotations.js from that directory. annotations.js has all the rooting hazard analysis annotations. The only way it is is used here is to ignore virtual calls through nsISupports::AddRef/Release, which kind of raises the issue of what this heap write analysis should do with nsISupports reference counts.

Awesome, that's a legit bug :) (filled as bug 1332915)

(In reply to Brian Hackett (:bhackett) from comment #18) > wants to try this themselves they need to build sixgill and will need this > file and a couple others I'll attach in a minute, plus the sixgill databases > for the tree they're analyzing. If necessary I can upload my databases from > a recent m-i somewhere. Note that you can get XDB files with a try push. Just include "--upload-xdbs" in the try syntax, and it'll put them in the upload directory (and dump out a link that you can access via job details in treeherder.)

(In reply to Brian Hackett (:bhackett) from comment #18) > Bobby, is this the sort of thing you had in mind? Yes, this is _exactly_ the kind of bug that I was looking to root out. Thank you! > I can suppress this one > with an annotation and keep cranking away at resolving errors Please do! >, or if anyone > wants to try this themselves they need to build sixgill and will need this > file and a couple others I'll attach in a minute, plus the sixgill databases > for the tree they're analyzing. If necessary I can upload my databases from > a recent m-i somewhere. If we can get the analysis fully passing, how difficult will it be to hook up and run on try like the rooting hazard analysis?

Cool, I'll keep working on this and just annotating (and making notes about) things that look suspicious. (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22) > If we can get the analysis fully passing, how difficult will it be to hook > up and run on try like the rooting hazard analysis? Well, it would be easy to integrate with the rooting hazard analysis itself, but that's not a good long term strategy. Steve knows everything about the tryserver and other release engineering integration.

This updates the analysis to add some more annotations and make it more sophisticated (see below). This is now able to successfully analyze all reachable code from Gecko_GetNextStyleChild. I'll attach a list of all the functions it encountered in a few minutes. When analyzing a function it now keeps track of which arguments/|this| variables should be considered threadsafe, and whose fields may be written to without causing an error (writing through pointers in those fields will still cause an error, as they should). This notion captures both stack pointers being passed to functions and the initial heap allocated structure passed to Gecko_GetNextStyleChild, and is propagated through to transitive callees. This is sufficient to characterize all writes in the reachable functions as safe, except for the bug above, another potential bug (see below), a couple of malloc library globals, and writes through the ExplicitChildIterator::mShadowIterator member, which is a thread local heap allocated structure that is annotated away for now. The only other major change I made was to improve the virtual call resolution code shared with the rooting hazard analysis to cope better with overloading. Previously, resolving a virtual call would produce all functions with that name in the various subclasses/superclasses. Now the resolving makes sure the number of arguments match up. I think this improvement is what has let the analysis ensure that no QIs are being performed (i.e. the ones I saw previously were due to bogus callgraph edges). Here is the only other error I saw which looked like a potential thread safety issue: Error: Field write nsIDocument.mCachedRootElement nsDocument.field:0 Location: mozilla::dom::Element* nsDocument::GetRootElementInternal() const @ dom/base/nsDocument.cpp:3916 Stack Trace: mozilla::dom::Element* nsIDocument::GetRootElement() const @ dom/base/nsDocument.cpp:3899 void mozilla::dom::AllChildrenIterator::AppendNativeAnonymousChildren() @ dom/base/ChildIterator.cpp:387 nsIContent* mozilla::dom::AllChildrenIterator::GetNextChild() @ dom/base/ChildIterator.cpp:433 nsIContent* mozilla::dom::StyleChildrenIterator::GetNextChild() @ dom/base/ChildIterator.cpp:593 Gecko_GetNextStyleChild @ layout/style/ServoBindings.cpp:159

Updated callgraph file with improved handling for overloaded virtual calls.

The 600 or so functions encountered by the analysis. The actual analysis runs in a little over a second on my machine, though loading type information beforehand takes 16 seconds (this only needs to be done once, even if additional root functions are analyzed). Also, I forgot above that writes to heap data under nsTArray::AppendElement also needed an annotation, though we know when we see the writes that the nsTArray itself is thread local. There are also several additional annotations for a functions that can do writes (memcpy, atomic intrinsics) and that want special callgraph handling (like MOZ_ReportCrash).

(In reply to Brian Hackett (:bhackett) from comment #23) > Cool, I'll keep working on this and just annotating (and making notes about) > things that look suspicious. > > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22) > > If we can get the analysis fully passing, how difficult will it be to hook > > up and run on try like the rooting hazard analysis? > > Well, it would be easy to integrate with the rooting hazard analysis itself, > but that's not a good long term strategy. Steve knows everything about the > tryserver and other release engineering integration. It may not be a good long term strategy, but that's exactly what I would do with this. Given how fast this analysis runs (especially the marginal cost over the rooting hazard analysis, for which we already need to load in the types), I can't really justify duplicating resources to split up the analyses. And the right long term solution would be to compile once, then share the XDB files, but they are huge and I'm guessing not all that binary diffable. Also, the current way things are set up makes it pretty easy to produce multiple outputs (but only one final status code, which will be somewhat confusing since people will now have to figure out *which* analysis broke). So at least for now, I'm planning on rolling this into the existing analysis job, with some work to make it clear which analysis is seeing problems. My main worry was if there were wildly different false positive rates, but it's sounding like that might work out surprisingly well. Just let me know when there's something I can wire up. (Or tell me if the existing stuff is good to go already?)

(In reply to Brian Hackett (:bhackett) from comment #24) > Error: Field write nsIDocument.mCachedRootElement nsDocument.field:0 > Location: mozilla::dom::Element* nsDocument::GetRootElementInternal() const > @ dom/base/nsDocument.cpp:3916 > Stack Trace: > mozilla::dom::Element* nsIDocument::GetRootElement() const @ > dom/base/nsDocument.cpp:3899 > void mozilla::dom::AllChildrenIterator::AppendNativeAnonymousChildren() @ > dom/base/ChildIterator.cpp:387 > nsIContent* mozilla::dom::AllChildrenIterator::GetNextChild() @ > dom/base/ChildIterator.cpp:433 > nsIContent* mozilla::dom::StyleChildrenIterator::GetNextChild() @ > dom/base/ChildIterator.cpp:593 > Gecko_GetNextStyleChild @ layout/style/ServoBindings.cpp:159 This is great! I've filed bug 1333183 to fix this issue.

(In reply to Brian Hackett (:bhackett) from comment #26) > Created attachment 8829492 [details] > visited functions > > The 600 or so functions encountered by the analysis. The actual analysis > runs in a little over a second on my machine, though loading type > information beforehand takes 16 seconds (this only needs to be done once, > even if additional root functions are analyzed). So far this just for Gecko_GetNextStyleChild, right? What's the best way to get this analysis for all the Gecko_ functions in ServoBindings.cpp (excluding the ones which MOZ_ASSERT(NS_IsMainThread()))?

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #29) > (In reply to Brian Hackett (:bhackett) from comment #26) > > Created attachment 8829492 [details] > > visited functions > > > > The 600 or so functions encountered by the analysis. The actual analysis > > runs in a little over a second on my machine, though loading type > > information beforehand takes 16 seconds (this only needs to be done once, > > even if additional root functions are analyzed). > > So far this just for Gecko_GetNextStyleChild, right? What's the best way to > get this analysis for all the Gecko_ functions in ServoBindings.cpp > (excluding the ones which MOZ_ASSERT(NS_IsMainThread()))? It should be pretty straightforward to just build this filter into the analysis. If we start ignoring paths where NS_IsMainThread returns true then we should get the last part of the filter here for free, at least in debug builds (which I'm guessing is what the rooting hazard analysis is looking at already). I'll work on this tomorrow.

(In reply to Brian Hackett (:bhackett) from comment #30) > If we start ignoring paths where NS_IsMainThread returns true > then we should get the last part of the filter here for free, at least in > debug builds (which I'm guessing is what the rooting hazard analysis is > looking at already). Yes, they're debug+opt, under the assumption that that would give the most code compiled and the trickiest reorderings (and similar).

Updated heap write analysis. I added an analysis to exclude code dominated by an NS_IsMainThread test, made some other small changes, and added a lot of annotations. The analysis is now able to process the 122 Gecko_* functions in ServoBindings.cpp. It would be nice to reduce the number of annotations required, which will help both with long term maintenance and confidence in the results of the analysis. There are a couple main improvements that would be nice: 1. Add a limited form of dataflow to the analysis that just tracks assignments between local variables. This would mainly help with code that allocates an object and then writes to its contents. 2. Have a way of identifying 'out' parameters for Servo bindings. Some parameters can be written into and others can't, and right now the ones that can all need annotations. This could be done either by using some sort of GCC attribute on 'out' parameters (doing this might need some improvements to sixgill, I'm not sure) or just using a naming idiom for the 'out' parameters like calling them aFooOut and such (would help with human readability too). Here is some suspicious code which the analysis turned up, with notes on a few of them: Error: AddRef/Release on nsISupports Location: nsCOMPtr<T>::~nsCOMPtr() [with T = nsIMozBrowserFrame] @ ff-dbg/dist/include/nsCOMPtr.h:404 Stack Trace: static mozilla::Maybe<bool> nsCSSPseudoClasses::MatchesElement(nsCSSPseudoClasses::Type, const mozilla::dom::Element*) @ layout/style/nsCSSPseudoClasses.cpp:153 Gecko_MatchesElement @ layout/style/ServoBindings.cpp:208 If nsIMozBrowserFrame always has threadsafe refcounts then this could be annotated away, but then I saw that nsCSSPseudoClasses::MatchesElement has a QueryInterface call (are the Servo bindings ever allowed to do these?) so I didn't get any further. Error: Field write nsStyleContext.mCachedResetData Location: void nsStyleContext::SetStyle(int32, void*) @ layout/style/nsStyleContext.cpp:600 Stack Trace: nsStyleEffects* nsStyleContext::DoGetStyleEffects() [with bool aComputeData = true] @ ff-dbg/dist/include/nsStyleStructList.h:151 nsStyleEffects* nsStyleContext::StyleEffects() @ ff-dbg/dist/include/nsStyleStructList.h:151 uint8 nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(nsStyleContext*) const [with StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:157 uint8 nsStyleDisplay::IsFixedPosContainingBlockForAppropriateFrame(nsStyleContext*) const [with StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:167 uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; nsStyleContext::NeutralChangeHandling aNeutralChangeHandling = (nsStyleContext::NeutralChangeHandling)0; uint32_t = unsigned int] @ layout/style/nsStyleContext.cpp:1211 uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1277 Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:280 Error: Field write MiscContainer.mStringBits Location: void nsAttrValue::SetMiscAtomOrString(nsAString_internal*) @ dom/base/nsAttrValue.cpp:1763 Stack Trace: void nsAttrValue::ToString(nsAString_internal*) const @ dom/base/nsAttrValue.cpp:650 uint8 nsAttrValue::Equals(nsIAtom*, uint32) const @ dom/base/nsAttrValue.cpp:1126 ServoBindings.cpp:uint8 AttrEquals(nsAttrValue*)::<lambda(const nsAttrValue*)> [with Implementor = const mozilla::dom::Element] @ layout/style/ServoBindings.cpp:385 ServoBindings.cpp:uint8 DoMatch(mozilla::dom::Element*, nsIAtom*, nsIAtom*, struct AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with Implementor = const mozilla::dom::Element]::<lambda(const class nsAttrValue*)>) [with Implementor = const mozilla::dom::Element; MatchFn = AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with Implementor = const mozilla::dom::Element]::<lambda(const nsAttrValue*)>] @ layout/style/ServoBindings.cpp:364 ServoBindings.cpp:uint8 AttrEquals(mozilla::dom::Element*, nsIAtom*, nsIAtom*, nsIAtom*, uint8) [with Implementor = const mozilla::dom::Element] @ layout/style/ServoBindings.cpp:387 Gecko_AttrEquals @ layout/style/ServoBindings.cpp:569 Error: Field write nsAutoRefCnt.mValue Location: uint64 nsAutoRefCnt::operator=(uint64) @ ff-dbg/dist/include/nsISupportsImpl.h:288 Stack Trace: uint32 mozilla::AnonymousCounterStyle::Release() @ layout/style/CounterStyleManager.h:136 static void mozilla::RefPtrTraits<U>::Release(U*) [with U = mozilla::CounterStyle] @ ff-dbg/dist/include/mozilla/RefPtr.h:40 static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = mozilla::CounterStyle; T = mozilla::CounterStyle] @ ff-dbg/dist/include/mozilla/RefPtr.h:394 void RefPtr<T>::assign_assuming_AddRef(T*) [with T = mozilla::CounterStyle] @ ff-dbg/dist/include/mozilla/RefPtr.h:65 void RefPtr<T>::assign_with_AddRef(T*) [with T = mozilla::CounterStyle] @ ff-dbg/dist/include/mozilla/RefPtr.h:56 RefPtr<T>& RefPtr<T>::operator=(T*) [with T = mozilla::CounterStyle] @ ff-dbg/dist/include/mozilla/RefPtr.h:191 void nsStyleList::SetCounterStyle(mozilla::CounterStyle*) @ layout/style/nsStyleStruct.h:1608 Gecko_SetListStyleType @ layout/style/ServoBindings.cpp:656 Error: Field write nsAutoRefCnt.mValue Location: uint64 nsAutoRefCnt::operator=(uint64) @ ff-dbg/dist/include/nsISupportsImpl.h:288 Stack Trace: uint32 mozilla::css::FontFamilyListRefCnt::Release() @ ff-dbg/dist/include/nsCSSValue.h:317 void nsCSSValue::DoReset() @ layout/style/nsCSSValue.cpp:444 void nsCSSValue::Reset() @ ff-dbg/dist/include/nsCSSValue.h:884 void nsCSSValue::SetFloatValue(float32, uint32) @ layout/style/nsCSSValue.cpp:473 void nsCSSValue::SetIntegerCoordValue(int32) @ layout/style/nsCSSValue.cpp:510 Gecko_CSSValue_SetAbsoluteLength @ layout/style/ServoBindings.cpp:1006 Error: AddRef/Release on nsISupports Location: nsCOMArray.cpp:void ReleaseObjects(class nsTArray<nsISupports*>*) @ xpcom/glue/nsCOMArray.cpp:272 Stack Trace: void nsCOMArray_base::Clear() @ xpcom/glue/nsCOMArray.cpp:281 void CachedBorderImageData::PurgeCachedImages() @ layout/style/nsStyleStruct.cpp:2044 void nsStyleImage::SetImageRequest(struct already_AddRefed<nsStyleImageRequest>) @ layout/style/nsStyleStruct.cpp:2150 Gecko_SetUrlImageValue @ layout/style/ServoBindings.cpp:752 Error: Field write nsCSSValue::Array.mRefCnt Location: void nsCSSValue::Array::AddRef() @ ff-dbg/dist/include/nsCSSValue.h:1071 Stack Trace: void nsStyleContentData::nsStyleContentData(nsStyleContentData*) @ layout/style/nsStyleStruct.cpp:3605 nsStyleContentData* nsStyleContentData::operator=(nsStyleContentData*) @ layout/style/nsStyleStruct.cpp:3620 Gecko_CopyStyleContentsFrom @ layout/style/ServoBindings.cpp:883 Error: Field write mozilla::css::SheetLoadData.mMustNotify Location: uint32 mozilla::css::Loader::LoadChildSheet(mozilla::StyleSheet*, nsIURI*, nsMediaList*, mozilla::css::ImportRule*, RawServoImportRule*, mozilla::css::LoaderReusableStyleSheets*) @ layout/style/Loader.cpp:2320 Stack Trace: Gecko_LoadStyleSheet @ layout/style/ServoBindings.cpp:1102 In this case the SheetLoadData being written was just allocated by the thread, so this write is safe (adding dataflow would help to avoid needing an annotation for this). However, LoadChildSheet does a QueryInterface and has a lot of other effectful-looking code, so I just suppressed the rest of its behavior in the analysis. Is all this code supposed to run on non-main-threads when called by Servo?

This is slightly different from the earlier version.

(In reply to Brian Hackett (:bhackett) from comment #32) > Error: AddRef/Release on nsISupports > Location: nsCOMPtr<T>::~nsCOMPtr() [with T = nsIMozBrowserFrame] @ > ff-dbg/dist/include/nsCOMPtr.h:404 > Stack Trace: > static mozilla::Maybe<bool> > nsCSSPseudoClasses::MatchesElement(nsCSSPseudoClasses::Type, const > mozilla::dom::Element*) @ layout/style/nsCSSPseudoClasses.cpp:153 > Gecko_MatchesElement @ layout/style/ServoBindings.cpp:208 > > If nsIMozBrowserFrame always has threadsafe refcounts then this could be > annotated away, but then I saw that nsCSSPseudoClasses::MatchesElement has a > QueryInterface call (are the Servo bindings ever allowed to do these?) so I > didn't get any further. Bug. > Error: Field write nsStyleContext.mCachedResetData > Location: void nsStyleContext::SetStyle(int32, void*) @ > layout/style/nsStyleContext.cpp:600 > Stack Trace: > nsStyleEffects* nsStyleContext::DoGetStyleEffects() [with bool aComputeData > = true] @ ff-dbg/dist/include/nsStyleStructList.h:151 > nsStyleEffects* nsStyleContext::StyleEffects() @ > ff-dbg/dist/include/nsStyleStructList.h:151 > uint8 > nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(nsStyleContext*) > const [with StyleContextLike = nsStyleContext] @ > layout/style/nsStyleStructInlines.h:157 > uint8 > nsStyleDisplay:: > IsFixedPosContainingBlockForAppropriateFrame(nsStyleContext*) const [with > StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:167 > uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, > uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; > nsStyleContext::NeutralChangeHandling aNeutralChangeHandling = > (nsStyleContext::NeutralChangeHandling)0; uint32_t = unsigned int] @ > layout/style/nsStyleContext.cpp:1211 > uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, > uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1277 > Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:280 This one is not a problem (it may be a bug in the analyzer) because we're calling the ServoComputedValues CalcStyleDifference overload, which uses a FakeStyleContext, which calls into Servo instead of going through DoGetStyleEffects (plus the style context is unique). That being said, Bobby, we should analyze with care that code for bugs in style comparation due to the `Peek` optimization. I believe in this case we're fine though. > Error: Field write MiscContainer.mStringBits > Location: void nsAttrValue::SetMiscAtomOrString(nsAString_internal*) @ > dom/base/nsAttrValue.cpp:1763 > Stack Trace: > void nsAttrValue::ToString(nsAString_internal*) const @ > dom/base/nsAttrValue.cpp:650 > uint8 nsAttrValue::Equals(nsIAtom*, uint32) const @ > dom/base/nsAttrValue.cpp:1126 > ServoBindings.cpp:uint8 AttrEquals(nsAttrValue*)::<lambda(const > nsAttrValue*)> [with Implementor = const mozilla::dom::Element] @ > layout/style/ServoBindings.cpp:385 > ServoBindings.cpp:uint8 DoMatch(mozilla::dom::Element*, nsIAtom*, nsIAtom*, > struct AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with > Implementor = const mozilla::dom::Element]::<lambda(const class > nsAttrValue*)>) [with Implementor = const mozilla::dom::Element; MatchFn = > AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with > Implementor = const mozilla::dom::Element]::<lambda(const nsAttrValue*)>] @ > layout/style/ServoBindings.cpp:364 > ServoBindings.cpp:uint8 AttrEquals(mozilla::dom::Element*, nsIAtom*, > nsIAtom*, nsIAtom*, uint8) [with Implementor = const mozilla::dom::Element] > @ layout/style/ServoBindings.cpp:387 > Gecko_AttrEquals @ layout/style/ServoBindings.cpp:569 Ugh, this would have been impossible to catch by hand. So this can only happen when there's a declaration block in an attribute[1], which I believe it's only the style attribute. We const-cast-and-mutate that nsAttrValue into a string on the fly, which feels pretty wrong to me. I don't know what's the reasoning for it though. [1]: http://searchfox.org/mozilla-central/source/dom/base/nsAttrValue.cpp#643 > Error: Field write nsAutoRefCnt.mValue > Location: uint64 nsAutoRefCnt::operator=(uint64) @ > ff-dbg/dist/include/nsISupportsImpl.h:288 > Stack Trace: > uint32 mozilla::AnonymousCounterStyle::Release() @ > layout/style/CounterStyleManager.h:136 > static void mozilla::RefPtrTraits<U>::Release(U*) [with U = > mozilla::CounterStyle] @ ff-dbg/dist/include/mozilla/RefPtr.h:40 > static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = > mozilla::CounterStyle; T = mozilla::CounterStyle] @ > ff-dbg/dist/include/mozilla/RefPtr.h:394 > void RefPtr<T>::assign_assuming_AddRef(T*) [with T = mozilla::CounterStyle] > @ ff-dbg/dist/include/mozilla/RefPtr.h:65 > void RefPtr<T>::assign_with_AddRef(T*) [with T = mozilla::CounterStyle] @ > ff-dbg/dist/include/mozilla/RefPtr.h:56 > RefPtr<T>& RefPtr<T>::operator=(T*) [with T = mozilla::CounterStyle] @ > ff-dbg/dist/include/mozilla/RefPtr.h:191 > void nsStyleList::SetCounterStyle(mozilla::CounterStyle*) @ > layout/style/nsStyleStruct.h:1608 > Gecko_SetListStyleType @ layout/style/ServoBindings.cpp:656 This seems to contradict the comment above that call, ouch. > // Builtin counter styles are static and use no-op refcounting, and thus are > // safe to use off-main-thread. > Error: Field write nsAutoRefCnt.mValue > Location: uint64 nsAutoRefCnt::operator=(uint64) @ > ff-dbg/dist/include/nsISupportsImpl.h:288 > Stack Trace: > uint32 mozilla::css::FontFamilyListRefCnt::Release() @ > ff-dbg/dist/include/nsCSSValue.h:317 > void nsCSSValue::DoReset() @ layout/style/nsCSSValue.cpp:444 > void nsCSSValue::Reset() @ ff-dbg/dist/include/nsCSSValue.h:884 > void nsCSSValue::SetFloatValue(float32, uint32) @ > layout/style/nsCSSValue.cpp:473 > void nsCSSValue::SetIntegerCoordValue(int32) @ > layout/style/nsCSSValue.cpp:510 > Gecko_CSSValue_SetAbsoluteLength @ layout/style/ServoBindings.cpp:1006 I believe this may be ok, because we only call this for other length properties (that can't contain a FontFamilyList). Worth looking more carefully though. > Error: AddRef/Release on nsISupports > Location: nsCOMArray.cpp:void ReleaseObjects(class nsTArray<nsISupports*>*) > @ xpcom/glue/nsCOMArray.cpp:272 > Stack Trace: > void nsCOMArray_base::Clear() @ xpcom/glue/nsCOMArray.cpp:281 > void CachedBorderImageData::PurgeCachedImages() @ > layout/style/nsStyleStruct.cpp:2044 > void nsStyleImage::SetImageRequest(struct > already_AddRefed<nsStyleImageRequest>) @ layout/style/nsStyleStruct.cpp:2150 > Gecko_SetUrlImageValue @ layout/style/ServoBindings.cpp:752 We only call these on fresh rust structs, so should be fine. > Error: Field write nsCSSValue::Array.mRefCnt > Location: void nsCSSValue::Array::AddRef() @ > ff-dbg/dist/include/nsCSSValue.h:1071 > Stack Trace: > void nsStyleContentData::nsStyleContentData(nsStyleContentData*) @ > layout/style/nsStyleStruct.cpp:3605 > nsStyleContentData* nsStyleContentData::operator=(nsStyleContentData*) @ > layout/style/nsStyleStruct.cpp:3620 > Gecko_CopyStyleContentsFrom @ layout/style/ServoBindings.cpp:883 I _believe_ we don't create Array values for the content() property, so should be ok. Worth also checking with more care. > Error: Field write mozilla::css::SheetLoadData.mMustNotify > Location: uint32 mozilla::css::Loader::LoadChildSheet(mozilla::StyleSheet*, > nsIURI*, nsMediaList*, mozilla::css::ImportRule*, RawServoImportRule*, > mozilla::css::LoaderReusableStyleSheets*) @ layout/style/Loader.cpp:2320 > Stack Trace: > Gecko_LoadStyleSheet @ layout/style/ServoBindings.cpp:1102 > In this case the SheetLoadData being written was just allocated by the > thread, so this write is safe (adding dataflow would help to avoid needing > an annotation for this). However, LoadChildSheet does a QueryInterface and > has a lot of other effectful-looking code, so I just suppressed the rest of > its behavior in the analysis. Is all this code supposed to run on > non-main-threads when called by Servo? This is only called on main thread during stylesheet parsing, so it's ok, you can ignore anything coming from LoadStyleSheet.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #34) > > Error: AddRef/Release on nsISupports > > Location: nsCOMArray.cpp:void ReleaseObjects(class nsTArray<nsISupports*>*) > > @ xpcom/glue/nsCOMArray.cpp:272 > > Stack Trace: > > void nsCOMArray_base::Clear() @ xpcom/glue/nsCOMArray.cpp:281 > > void CachedBorderImageData::PurgeCachedImages() @ > > layout/style/nsStyleStruct.cpp:2044 > > void nsStyleImage::SetImageRequest(struct > > already_AddRefed<nsStyleImageRequest>) @ layout/style/nsStyleStruct.cpp:2150 > > Gecko_SetUrlImageValue @ layout/style/ServoBindings.cpp:752 > > We only call these on fresh rust structs, so should be fine. Nope, I was wrong, of course the side-effect is the refcount drop, this is not fine at all I fear :) We should file bugs for the one that aren't clearly right, I need to run for today though.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #34) > (In reply to Brian Hackett (:bhackett) from comment #32) > > Error: AddRef/Release on nsISupports > > Location: nsCOMPtr<T>::~nsCOMPtr() [with T = nsIMozBrowserFrame] @ > > ff-dbg/dist/include/nsCOMPtr.h:404 > > Stack Trace: > > static mozilla::Maybe<bool> > > nsCSSPseudoClasses::MatchesElement(nsCSSPseudoClasses::Type, const > > mozilla::dom::Element*) @ layout/style/nsCSSPseudoClasses.cpp:153 > > Gecko_MatchesElement @ layout/style/ServoBindings.cpp:208 > > > > If nsIMozBrowserFrame always has threadsafe refcounts then this could be > > annotated away, but then I saw that nsCSSPseudoClasses::MatchesElement has a > > QueryInterface call (are the Servo bindings ever allowed to do these?) so I > > didn't get any further. > > Bug. Filed bug 1335303. > > > Error: Field write nsStyleContext.mCachedResetData > > Location: void nsStyleContext::SetStyle(int32, void*) @ > > layout/style/nsStyleContext.cpp:600 > > Stack Trace: > > nsStyleEffects* nsStyleContext::DoGetStyleEffects() [with bool aComputeData > > = true] @ ff-dbg/dist/include/nsStyleStructList.h:151 > > nsStyleEffects* nsStyleContext::StyleEffects() @ > > ff-dbg/dist/include/nsStyleStructList.h:151 > > uint8 > > nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(nsStyleContext*) > > const [with StyleContextLike = nsStyleContext] @ > > layout/style/nsStyleStructInlines.h:157 > > uint8 > > nsStyleDisplay:: > > IsFixedPosContainingBlockForAppropriateFrame(nsStyleContext*) const [with > > StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:167 > > uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, > > uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; > > nsStyleContext::NeutralChangeHandling aNeutralChangeHandling = > > (nsStyleContext::NeutralChangeHandling)0; uint32_t = unsigned int] @ > > layout/style/nsStyleContext.cpp:1211 > > uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, > > uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1277 > > Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:280 > > This one is not a problem (it may be a bug in the analyzer) because we're > calling the ServoComputedValues CalcStyleDifference overload, which uses a > FakeStyleContext, which calls into Servo instead of going through > DoGetStyleEffects (plus the style context is unique). We only use FakeStyleContext for the new style context - the old context is still real. We probably have exclusive access to the style context here, but I think it's better to be safe. Filed bug 1335317. > > Error: Field write MiscContainer.mStringBits > > Location: void nsAttrValue::SetMiscAtomOrString(nsAString_internal*) @ > > dom/base/nsAttrValue.cpp:1763 > > Stack Trace: > > void nsAttrValue::ToString(nsAString_internal*) const @ > > dom/base/nsAttrValue.cpp:650 > > uint8 nsAttrValue::Equals(nsIAtom*, uint32) const @ > > dom/base/nsAttrValue.cpp:1126 > > ServoBindings.cpp:uint8 AttrEquals(nsAttrValue*)::<lambda(const > > nsAttrValue*)> [with Implementor = const mozilla::dom::Element] @ > > layout/style/ServoBindings.cpp:385 > > ServoBindings.cpp:uint8 DoMatch(mozilla::dom::Element*, nsIAtom*, nsIAtom*, > > struct AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with > > Implementor = const mozilla::dom::Element]::<lambda(const class > > nsAttrValue*)>) [with Implementor = const mozilla::dom::Element; MatchFn = > > AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with > > Implementor = const mozilla::dom::Element]::<lambda(const nsAttrValue*)>] @ > > layout/style/ServoBindings.cpp:364 > > ServoBindings.cpp:uint8 AttrEquals(mozilla::dom::Element*, nsIAtom*, > > nsIAtom*, nsIAtom*, uint8) [with Implementor = const mozilla::dom::Element] > > @ layout/style/ServoBindings.cpp:387 > > Gecko_AttrEquals @ layout/style/ServoBindings.cpp:569 > > Ugh, this would have been impossible to catch by hand. So this can only > happen when there's a declaration block in an attribute[1], which I believe > it's only the style attribute. > > We const-cast-and-mutate that nsAttrValue into a string on the fly, which > feels pretty wrong to me. I don't know what's the reasoning for it though. Filed bug 1335305. > > [1]: http://searchfox.org/mozilla-central/source/dom/base/nsAttrValue.cpp#643 > > > Error: Field write nsAutoRefCnt.mValue > > Location: uint64 nsAutoRefCnt::operator=(uint64) @ > > ff-dbg/dist/include/nsISupportsImpl.h:288 > > Stack Trace: > > uint32 mozilla::AnonymousCounterStyle::Release() @ > > layout/style/CounterStyleManager.h:136 > > static void mozilla::RefPtrTraits<U>::Release(U*) [with U = > > mozilla::CounterStyle] @ ff-dbg/dist/include/mozilla/RefPtr.h:40 > > static void RefPtr<T>::ConstRemovingRefPtrTraits<U>::Release(U*) [with U = > > mozilla::CounterStyle; T = mozilla::CounterStyle] @ > > ff-dbg/dist/include/mozilla/RefPtr.h:394 > > void RefPtr<T>::assign_assuming_AddRef(T*) [with T = mozilla::CounterStyle] > > @ ff-dbg/dist/include/mozilla/RefPtr.h:65 > > void RefPtr<T>::assign_with_AddRef(T*) [with T = mozilla::CounterStyle] @ > > ff-dbg/dist/include/mozilla/RefPtr.h:56 > > RefPtr<T>& RefPtr<T>::operator=(T*) [with T = mozilla::CounterStyle] @ > > ff-dbg/dist/include/mozilla/RefPtr.h:191 > > void nsStyleList::SetCounterStyle(mozilla::CounterStyle*) @ > > layout/style/nsStyleStruct.h:1608 > > Gecko_SetListStyleType @ layout/style/ServoBindings.cpp:656 > > This seems to contradict the comment above that call, ouch. > > > // Builtin counter styles are static and use no-op refcounting, and thus are > > // safe to use off-main-thread. I think the comment is still correct. The callsite only passes builtin counter styles, and none of those correspond to AnonymousCounterStyle. We should whitelist this for now I think. > > > > Error: Field write nsAutoRefCnt.mValue > > Location: uint64 nsAutoRefCnt::operator=(uint64) @ > > ff-dbg/dist/include/nsISupportsImpl.h:288 > > Stack Trace: > > uint32 mozilla::css::FontFamilyListRefCnt::Release() @ > > ff-dbg/dist/include/nsCSSValue.h:317 > > void nsCSSValue::DoReset() @ layout/style/nsCSSValue.cpp:444 > > void nsCSSValue::Reset() @ ff-dbg/dist/include/nsCSSValue.h:884 > > void nsCSSValue::SetFloatValue(float32, uint32) @ > > layout/style/nsCSSValue.cpp:473 > > void nsCSSValue::SetIntegerCoordValue(int32) @ > > layout/style/nsCSSValue.cpp:510 > > Gecko_CSSValue_SetAbsoluteLength @ layout/style/ServoBindings.cpp:1006 > > I believe this may be ok, because we only call this for other length > properties (that can't contain a FontFamilyList). Worth looking more > carefully though. bug 1335308. > > > Error: AddRef/Release on nsISupports > > Location: nsCOMArray.cpp:void ReleaseObjects(class nsTArray<nsISupports*>*) > > @ xpcom/glue/nsCOMArray.cpp:272 > > Stack Trace: > > void nsCOMArray_base::Clear() @ xpcom/glue/nsCOMArray.cpp:281 > > void CachedBorderImageData::PurgeCachedImages() @ > > layout/style/nsStyleStruct.cpp:2044 > > void nsStyleImage::SetImageRequest(struct > > already_AddRefed<nsStyleImageRequest>) @ layout/style/nsStyleStruct.cpp:2150 > > Gecko_SetUrlImageValue @ layout/style/ServoBindings.cpp:752 bug 1335321. > > > Error: Field write nsCSSValue::Array.mRefCnt > > Location: void nsCSSValue::Array::AddRef() @ > > ff-dbg/dist/include/nsCSSValue.h:1071 > > Stack Trace: > > void nsStyleContentData::nsStyleContentData(nsStyleContentData*) @ > > layout/style/nsStyleStruct.cpp:3605 > > nsStyleContentData* nsStyleContentData::operator=(nsStyleContentData*) @ > > layout/style/nsStyleStruct.cpp:3620 > > Gecko_CopyStyleContentsFrom @ layout/style/ServoBindings.cpp:883 > > I _believe_ we don't create Array values for the content() property, so > should be ok. Worth also checking with more care. Can you explain your reasoning here? Why would this kind of value exist if it weren't used? > > > Error: Field write mozilla::css::SheetLoadData.mMustNotify > > Location: uint32 mozilla::css::Loader::LoadChildSheet(mozilla::StyleSheet*, > > nsIURI*, nsMediaList*, mozilla::css::ImportRule*, RawServoImportRule*, > > mozilla::css::LoaderReusableStyleSheets*) @ layout/style/Loader.cpp:2320 > > Stack Trace: > > Gecko_LoadStyleSheet @ layout/style/ServoBindings.cpp:1102 > > In this case the SheetLoadData being written was just allocated by the > > thread, so this write is safe (adding dataflow would help to avoid needing > > an annotation for this). However, LoadChildSheet does a QueryInterface and > > has a lot of other effectful-looking code, so I just suppressed the rest of > > its behavior in the analysis. Is all this code supposed to run on > > non-main-threads when called by Servo? > > This is only called on main thread during stylesheet parsing, so it's ok, > you can ignore anything coming from LoadStyleSheet. bug 1335308.

NI emilio for the question in comment 37.

> Can you explain your reasoning here? Why would this kind of value exist if it weren't used? Whoops, sorry for missing this one. So I was talking from memory here, but I was right in that we don't create them (if only because it's unsupported yet, along with other stuff like img(..) and attr(..)): https://github.com/servo/servo/blob/master/components/style/properties/gecko.mako.rs#L2772 We'll need to refactor quite a bit how content works to support those I believe. Worth having this into account, but not a thread-safety issue right now.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #39) > We'll need to refactor quite a bit how content works to support those I > believe. Worth having this into account, but not a thread-safety issue right > now. Ok, I'll assert against it for now then in bug 1335308.

Brian, I think all the issues you found are now fixed except for bug 1335321. Are there more? If not, is this stuff in good enough shape to hand to sfink and get running on CI?

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #41) > Brian, I think all the issues you found are now fixed except for bug > 1335321. Are there more? If not, is this stuff in good enough shape to hand > to sfink and get running on CI? Awesome, I'll update and run the analysis again and get rid of as many annotations as I can (I want to add a primitive local-variable-assignment dataflow to help with this). I should have an updated analysis by tomorrow.

Updated analysis with better tracking of local variables through assignments and some other improvements. Below are some suspicious accesses I ran into while running the analysis again. These might be new issues, or might have just been masked by annotations I used to suppress the earlier issues. Error: Field write nsPrincipal.mInitialized Location: uint32 nsPrincipal::Init(nsIURI*, mozilla::OriginAttributes*) @ caps/nsPrincipal.cpp:85 ### SafeArguments: arg1 Stack Trace: static already_AddRefed<mozilla::BasePrincipal> mozilla::BasePrincipal::CreateCodebasePrincipal(nsIURI*, const mozilla::OriginAttributes&) @ caps/BasePrincipal.cpp:687 ### SafeArguments: arg1 nsPermissionManager::PermissionHashKey* nsPermissionManager::GetPermissionHashKey(nsIPrincipal*, uint32, uint8) @ extensions/cookie/nsPermissionManager.cpp:2221 ### SafeArguments: arg2 uint32 nsPermissionManager::CommonTestPermission(nsIPrincipal*, int8*, uint32*, uint8, uint8) @ extensions/cookie/nsPermissionManager.cpp:2127 ### SafeArguments: arg2 arg3 uint32 nsPermissionManager::TestPermissionFromPrincipal(nsIPrincipal*, int8*, uint32*) @ extensions/cookie/nsPermissionManager.cpp:2009 ### SafeArguments: arg2 uint32 nsGenericHTMLFrameElement::GetReallyIsBrowser(uint8*) @ dom/html/nsGenericHTMLFrameElement.cpp:508 ### SafeArguments: arg0 uint8 nsIMozBrowserFrame::GetReallyIsBrowser() @ ff-dbg/dist/include/nsIMozBrowserFrame.h:40 static mozilla::Maybe<bool> nsCSSPseudoClasses::MatchesElement(nsCSSPseudoClasses::Type, const mozilla::dom::Element*) @ layout/style/nsCSSPseudoClasses.cpp:153 Gecko_MatchesElement @ layout/style/ServoBindings.cpp:221 Error: Dereference write sresult Location: uint32 nsComponentManagerImpl::GetServiceByContractID(int8*, nsID*, void**) @ xpcom/components/nsComponentManager.cpp:1522 ### SafeArguments: arg2 Stack Trace: uint32 CallGetService(int8*, nsID*, void**) @ xpcom/components/nsComponentManagerUtils.cpp:67 ### SafeArguments: arg2 uint32 nsGetServiceByContractID::operator(nsID*, void**)(const nsIID&, void**) const @ xpcom/components/nsComponentManagerUtils.cpp:280 ### SafeArguments: this arg1 void nsCOMPtr<T>::assign_from_gs_contractid(nsGetServiceByContractID, const nsIID&) [with T = nsIPermissionManager; nsIID = nsID] @ ff-dbg/dist/include/nsCOMPtr.h:1162 ### SafeArguments: this nsCOMPtr<T>::nsCOMPtr(nsGetServiceByContractID) [with T = nsIPermissionManager] @ ff-dbg/dist/include/nsCOMPtr.h:555 ### SafeArguments: this already_AddRefed<nsIPermissionManager> mozilla::services::GetPermissionManager() @ xpcom/build/ServiceList.h:26 uint32 nsGenericHTMLFrameElement::GetReallyIsBrowser(uint8*) @ dom/html/nsGenericHTMLFrameElement.cpp:504 ### SafeArguments: arg0 uint8 nsIMozBrowserFrame::GetReallyIsBrowser() @ ff-dbg/dist/include/nsIMozBrowserFrame.h:40 static mozilla::Maybe<bool> nsCSSPseudoClasses::MatchesElement(nsCSSPseudoClasses::Type, const mozilla::dom::Element*) @ layout/style/nsCSSPseudoClasses.cpp:153 Gecko_MatchesElement @ layout/style/ServoBindings.cpp:221 Error: Field write nsStyleContext.mCachedResetData Location: void nsStyleContext::SetStyle(int32, void*) @ layout/style/nsStyleContext.cpp:600 Stack Trace: nsStyleDisplay* nsStyleContext::DoGetStyleDisplay() [with bool aComputeData = true] @ ff-dbg/dist/include/nsStyleStructList.h:98 nsStyleDisplay* nsStyleContext::StyleDisplay() @ ff-dbg/dist/include/nsStyleStructList.h:98 uint8 nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(nsStyleContext*) const [with StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:154 uint8 nsStyleDisplay::IsFixedPosContainingBlockForAppropriateFrame(nsStyleContext*) const [with StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:184 uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; uint32_t = unsigned int] @ layout/style/nsStyleContext.cpp:1205 ### SafeArguments: arg0 arg2 arg3 uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1265 ### SafeArguments: arg2 arg3 Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:293 This particular stack trace is inside an NS_Assertion, but CalcStyleDifferenceInternal calls StyleDisplay() directly too. Error: Dereference write __temp_1 Location: void mozilla::EffectSet::UpdateAnimationRuleRefreshTime(uint32, mozilla::TimeStamp*) @ ff-dbg/dist/include/mozilla/EffectSet.h:181 ### SafeArguments: arg1 Stack Trace: mozilla::ServoAnimationRule* mozilla::EffectCompositor::GetServoAnimationRule(mozilla::dom::Element*, uint8, uint32) @ dom/animation/EffectCompositor.cpp:498 Gecko_GetAnimationRule @ layout/style/ServoBindings.cpp:385 If Gecko_GetAnimationRule is allowed to write to its element parameter this is fine I think, though it's hard to tell whether this should be considered safe. It would be really nice --- for this and the reasons I described earlier --- if there was a naming convention or some other mechanism for identifying out parameters in Servo bindings. Error: Field write nsRefreshDriver.mActiveTimer Location: void nsRefreshDriver::EnsureTimerStarted(uint32) @ layout/base/nsRefreshDriver.cpp:1302 ### SafeArguments: arg0 Stack Trace: mozilla::TimeStamp nsRefreshDriver::MostRecentRefresh() const @ layout/base/nsRefreshDriver.cpp:1187 mozilla::ServoAnimationRule* mozilla::EffectCompositor::GetServoAnimationRule(mozilla::dom::Element*, uint8, uint32) @ dom/animation/EffectCompositor.cpp:498 Gecko_GetAnimationRule @ layout/style/ServoBindings.cpp:385 Error: Field write mozilla::DeclarationBlock.mImmutable Location: void mozilla::DeclarationBlock::SetImmutable() const @ ff-dbg/dist/include/mozilla/DeclarationBlock.h:66 Stack Trace: void mozilla::css::Declaration::ToString(nsAString_internal*) const @ layout/style/Declaration.cpp:1706 ### SafeArguments: arg0 void mozilla::DeclarationBlock::ToString(nsAString_internal*) const @ ff-dbg/dist/include/mozilla/DeclarationBlockInlines.h:58 ### SafeArguments: arg0 void nsAttrValue::ToString(nsAString_internal*) const @ dom/base/nsAttrValue.cpp:649 ### SafeArguments: arg0 uint8 nsAttrValue::Equals(nsIAtom*, uint32) const @ dom/base/nsAttrValue.cpp:1135 ServoBindings.cpp:uint8 AttrEquals(nsAttrValue*)::<lambda(const nsAttrValue*)> [with Implementor = const mozilla::dom::Element] @ layout/style/ServoBindings.cpp:451 ### SafeArguments: this ServoBindings.cpp:uint8 DoMatch(mozilla::dom::Element*, nsIAtom*, nsIAtom*, struct AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with Implementor = const mozilla::dom::Element]::<lambda(const class nsAttrValue*)>) [with Implementor = const mozilla::dom::Element; MatchFn = AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with Implementor = const mozilla::dom::Element]::<lambda(const nsAttrValue*)>] @ layout/style/ServoBindings.cpp:430 ServoBindings.cpp:uint8 AttrEquals(mozilla::dom::Element*, nsIAtom*, nsIAtom*, nsIAtom*, uint8) [with Implementor = const mozilla::dom::Element] @ layout/style/ServoBindings.cpp:453 Gecko_AttrEquals @ layout/style/ServoBindings.cpp:635

(In reply to Brian Hackett (:bhackett) from comment #43) > Created attachment 8836248 [details] > analyzeHeapWrites.js > > Updated analysis with better tracking of local variables through assignments > and some other improvements. Below are some suspicious accesses I ran into > while running the analysis again. These might be new issues, or might have > just been masked by annotations I used to suppress the earlier issues. > > Error: Field write nsPrincipal.mInitialized > Location: uint32 nsPrincipal::Init(nsIURI*, mozilla::OriginAttributes*) @ > caps/nsPrincipal.cpp:85 ### SafeArguments: arg1 > Stack Trace: > static already_AddRefed<mozilla::BasePrincipal> > mozilla::BasePrincipal::CreateCodebasePrincipal(nsIURI*, const > mozilla::OriginAttributes&) @ caps/BasePrincipal.cpp:687 ### SafeArguments: > arg1 > nsPermissionManager::PermissionHashKey* > nsPermissionManager::GetPermissionHashKey(nsIPrincipal*, uint32, uint8) @ > extensions/cookie/nsPermissionManager.cpp:2221 ### SafeArguments: arg2 > uint32 nsPermissionManager::CommonTestPermission(nsIPrincipal*, int8*, > uint32*, uint8, uint8) @ extensions/cookie/nsPermissionManager.cpp:2127 ### > SafeArguments: arg2 arg3 > uint32 nsPermissionManager::TestPermissionFromPrincipal(nsIPrincipal*, > int8*, uint32*) @ extensions/cookie/nsPermissionManager.cpp:2009 ### > SafeArguments: arg2 > uint32 nsGenericHTMLFrameElement::GetReallyIsBrowser(uint8*) @ > dom/html/nsGenericHTMLFrameElement.cpp:508 ### SafeArguments: arg0 > uint8 nsIMozBrowserFrame::GetReallyIsBrowser() @ > ff-dbg/dist/include/nsIMozBrowserFrame.h:40 > static mozilla::Maybe<bool> > nsCSSPseudoClasses::MatchesElement(nsCSSPseudoClasses::Type, const > mozilla::dom::Element*) @ layout/style/nsCSSPseudoClasses.cpp:153 > Gecko_MatchesElement @ layout/style/ServoBindings.cpp:221 > > Error: Dereference write sresult > Location: uint32 nsComponentManagerImpl::GetServiceByContractID(int8*, > nsID*, void**) @ xpcom/components/nsComponentManager.cpp:1522 ### > SafeArguments: arg2 > Stack Trace: > uint32 CallGetService(int8*, nsID*, void**) @ > xpcom/components/nsComponentManagerUtils.cpp:67 ### SafeArguments: arg2 > uint32 nsGetServiceByContractID::operator(nsID*, void**)(const nsIID&, > void**) const @ xpcom/components/nsComponentManagerUtils.cpp:280 ### > SafeArguments: this arg1 > void nsCOMPtr<T>::assign_from_gs_contractid(nsGetServiceByContractID, const > nsIID&) [with T = nsIPermissionManager; nsIID = nsID] @ > ff-dbg/dist/include/nsCOMPtr.h:1162 ### SafeArguments: this > nsCOMPtr<T>::nsCOMPtr(nsGetServiceByContractID) [with T = > nsIPermissionManager] @ ff-dbg/dist/include/nsCOMPtr.h:555 ### > SafeArguments: this > already_AddRefed<nsIPermissionManager> > mozilla::services::GetPermissionManager() @ xpcom/build/ServiceList.h:26 > uint32 nsGenericHTMLFrameElement::GetReallyIsBrowser(uint8*) @ > dom/html/nsGenericHTMLFrameElement.cpp:504 ### SafeArguments: arg0 > uint8 nsIMozBrowserFrame::GetReallyIsBrowser() @ > ff-dbg/dist/include/nsIMozBrowserFrame.h:40 > static mozilla::Maybe<bool> > nsCSSPseudoClasses::MatchesElement(nsCSSPseudoClasses::Type, const > mozilla::dom::Element*) @ layout/style/nsCSSPseudoClasses.cpp:153 > Gecko_MatchesElement @ layout/style/ServoBindings.cpp:221 Bug 1340333 should take care of these. > > Error: Field write nsStyleContext.mCachedResetData > Location: void nsStyleContext::SetStyle(int32, void*) @ > layout/style/nsStyleContext.cpp:600 > Stack Trace: > nsStyleDisplay* nsStyleContext::DoGetStyleDisplay() [with bool aComputeData > = true] @ ff-dbg/dist/include/nsStyleStructList.h:98 > nsStyleDisplay* nsStyleContext::StyleDisplay() @ > ff-dbg/dist/include/nsStyleStructList.h:98 > uint8 > nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(nsStyleContext*) > const [with StyleContextLike = nsStyleContext] @ > layout/style/nsStyleStructInlines.h:154 > uint8 > nsStyleDisplay:: > IsFixedPosContainingBlockForAppropriateFrame(nsStyleContext*) const [with > StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:184 > uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, > uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; > uint32_t = unsigned int] @ layout/style/nsStyleContext.cpp:1205 ### > SafeArguments: arg0 arg2 arg3 > uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, > uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1265 ### SafeArguments: > arg2 arg3 > Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:293 > > This particular stack trace is inside an NS_Assertion, but > CalcStyleDifferenceInternal calls StyleDisplay() directly too. bug 1340339. > > Error: Dereference write __temp_1 > Location: void mozilla::EffectSet::UpdateAnimationRuleRefreshTime(uint32, > mozilla::TimeStamp*) @ ff-dbg/dist/include/mozilla/EffectSet.h:181 ### > SafeArguments: arg1 > Stack Trace: > mozilla::ServoAnimationRule* > mozilla::EffectCompositor::GetServoAnimationRule(mozilla::dom::Element*, > uint8, uint32) @ dom/animation/EffectCompositor.cpp:498 > Gecko_GetAnimationRule @ layout/style/ServoBindings.cpp:385 > > If Gecko_GetAnimationRule is allowed to write to its element parameter That looks fishy to me. Filed bug 1340340. > this > is fine I think, though it's hard to tell whether this should be considered > safe. It would be really nice --- for this and the reasons I described > earlier --- if there was a naming convention or some other mechanism for > identifying out parameters in Servo bindings. > > Error: Field write nsRefreshDriver.mActiveTimer > Location: void nsRefreshDriver::EnsureTimerStarted(uint32) @ > layout/base/nsRefreshDriver.cpp:1302 ### SafeArguments: arg0 > Stack Trace: > mozilla::TimeStamp nsRefreshDriver::MostRecentRefresh() const @ > layout/base/nsRefreshDriver.cpp:1187 > mozilla::ServoAnimationRule* > mozilla::EffectCompositor::GetServoAnimationRule(mozilla::dom::Element*, > uint8, uint32) @ dom/animation/EffectCompositor.cpp:498 > Gecko_GetAnimationRule @ layout/style/ServoBindings.cpp:385 We should also fix this in bug 1340340. > > Error: Field write mozilla::DeclarationBlock.mImmutable > Location: void mozilla::DeclarationBlock::SetImmutable() const @ > ff-dbg/dist/include/mozilla/DeclarationBlock.h:66 > Stack Trace: > void mozilla::css::Declaration::ToString(nsAString_internal*) const @ > layout/style/Declaration.cpp:1706 ### SafeArguments: arg0 > void mozilla::DeclarationBlock::ToString(nsAString_internal*) const @ > ff-dbg/dist/include/mozilla/DeclarationBlockInlines.h:58 ### SafeArguments: > arg0 > void nsAttrValue::ToString(nsAString_internal*) const @ > dom/base/nsAttrValue.cpp:649 ### SafeArguments: arg0 > uint8 nsAttrValue::Equals(nsIAtom*, uint32) const @ > dom/base/nsAttrValue.cpp:1135 > ServoBindings.cpp:uint8 AttrEquals(nsAttrValue*)::<lambda(const > nsAttrValue*)> [with Implementor = const mozilla::dom::Element] @ > layout/style/ServoBindings.cpp:451 ### SafeArguments: this > ServoBindings.cpp:uint8 DoMatch(mozilla::dom::Element*, nsIAtom*, nsIAtom*, > struct AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with > Implementor = const mozilla::dom::Element]::<lambda(const class > nsAttrValue*)>) [with Implementor = const mozilla::dom::Element; MatchFn = > AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with > Implementor = const mozilla::dom::Element]::<lambda(const nsAttrValue*)>] @ > layout/style/ServoBindings.cpp:430 > ServoBindings.cpp:uint8 AttrEquals(mozilla::dom::Element*, nsIAtom*, > nsIAtom*, nsIAtom*, uint8) [with Implementor = const mozilla::dom::Element] > @ layout/style/ServoBindings.cpp:453 > Gecko_AttrEquals @ layout/style/ServoBindings.cpp:635 bug 1340341.

I think this is ready for another go-round, ignoring the animation function for now.

Updated analysis. Here are the suspicious accesses which this analysis turned up on a current m-i: Error: Field write nsFrameManagerBase::UndisplayedMap.mLastLookup Location: PLHashEntry** nsFrameManagerBase::UndisplayedMap::GetEntryFor(nsIContent**) @ layout/base/nsFrameManager.cpp:731 ### SafeArguments: arg0 Stack Trace: mozilla::UndisplayedNode* nsFrameManagerBase::UndisplayedMap::GetFirstNode(nsIContent*) @ layout/base/nsFrameManager.cpp:739 mozilla::UndisplayedNode* nsFrameManager::GetUndisplayedNodeInMapFor(nsFrameManagerBase::UndisplayedMap*, nsIContent*) @ layout/base/nsFrameManager.cpp:236 nsStyleContext* nsFrameManager::GetStyleContextInMap(nsFrameManagerBase::UndisplayedMap*, nsIContent*) @ layout/base/nsFrameManager.cpp:224 nsStyleContext* nsFrameManager::GetDisplayContentsStyleFor(nsIContent*) @ layout/base/nsFrameManager.h:135 Gecko_GetStyleContext @ layout/style/ServoBindings.cpp:297 Error: Field write nsStyleContext.mBits Location: void nsStyleContext::AddStyleBit(uint64*) @ layout/style/nsStyleContext.h:321 ### SafeArguments: arg0 Stack Trace: nsStyleDisplay* nsStyleContext::DoGetStyleDisplay() [with bool aComputeData = true] @ ff-dbg/dist/include/nsStyleStructList.h:98 nsStyleDisplay* nsStyleContext::StyleDisplay() @ ff-dbg/dist/include/nsStyleStructList.h:98 uint8 nsStyleDisplay::HasFixedPosContainingBlockStyleInternal(nsStyleContext*) const [with StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:154 uint8 nsStyleDisplay::IsFixedPosContainingBlockForAppropriateFrame(nsStyleContext*) const [with StyleContextLike = nsStyleContext] @ layout/style/nsStyleStructInlines.h:175 uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; uint32_t = unsigned int] @ layout/style/nsStyleContext.cpp:1248 ### SafeArguments: arg0 arg2 arg3 uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1311 ### SafeArguments: arg2 arg3 Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:316 Error: Dereference write __temp_4 Location: nsStyleUserInterface* nsStyleContext::DoGetStyleUserInterface() [with bool aComputeData = true] @ ff-dbg/dist/include/nsStyleStructList.h:65 Stack Trace: nsStyleUserInterface* nsStyleContext::StyleUserInterface() @ ff-dbg/dist/include/nsStyleStructList.h:65 uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; uint32_t = unsigned int] @ layout/style/nsCSSVisitedDependentPropList.h:36 ### SafeArguments: arg0 arg2 arg3 uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1311 ### SafeArguments: arg2 arg3 Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:316 Between the __temp_4 and the macros involved here it's hard to tell that it's complaining about the write through mCachedInheritedData at nsStyleContext.h:630. There are some other DoGetStyle* functions which are also called on similar stacks. Error: AddRef/Release on nsISupports Location: nsCOMPtr<T>::~nsCOMPtr() [with T = nsIURIWithPrincipal] @ ff-dbg/dist/include/nsCOMPtr.h:404 ### SafeArguments: this Stack Trace: uint8 NS_SecurityCompareURIs(nsIURI*, nsIURI*, uint8) @ netwerk/base/nsNetUtil.cpp:1694 uint8 nsScriptSecurityManager::SecurityCompareURIs(nsIURI*, nsIURI*) @ caps/nsScriptSecurityManager.cpp:232 uint8 nsPrincipal::SubsumesInternal(nsIPrincipal*, uint32) @ caps/nsPrincipal.cpp:239 ### SafeArguments: arg1 uint8 mozilla::BasePrincipal::Subsumes(nsIPrincipal*, uint32) @ caps/BasePrincipal.cpp:340 ### SafeArguments: arg1 uint32 mozilla::BasePrincipal::Equals(nsIPrincipal*, uint8*) @ caps/BasePrincipal.cpp:348 ### SafeArguments: arg1 uint8 nsCSSValueTokenStream::operator==(nsCSSValueTokenStream*) const @ ff-dbg/dist/include/nsCSSValue.h:1780 uint8 nsCSSValue::operator==(nsCSSValue*) const @ layout/style/nsCSSValue.cpp:297 uint8 nsCSSValue::operator!=(nsCSSValue*) const @ ff-dbg/dist/include/nsCSSValue.h:616 uint8 nsCSSValue::Array::operator==(nsCSSValue::Array*) const @ ff-dbg/dist/include/nsCSSValue.h:1061 uint8 nsStyleContentData::operator==(nsStyleContentData*) const @ layout/style/nsStyleStruct.cpp:3698 bool nsTArray_Impl<E, Alloc>::operator==(const nsTArray_Impl<E, Allocator>&) const [with Allocator = nsTArrayInfallibleAllocator; E = nsStyleContentData; Alloc = nsTArrayInfallibleAllocator] @ ff-dbg/dist/include/nsTArray.h:1088 bool nsTArray_Impl<E, Alloc>::operator!=(const self_type&) const [with E = nsStyleContentData; Alloc = nsTArrayInfallibleAllocator; nsTArray_Impl<E, Alloc>::self_type = nsTArray_Impl<nsStyleContentData, nsTArrayInfallibleAllocator>] @ ff-dbg/dist/include/nsTArray.h:1098 uint32 nsStyleContent::CalcDifference(nsStyleContent*) const @ layout/style/nsStyleStruct.cpp:3751 uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; uint32_t = unsigned int] @ layout/style/nsStyleContext.cpp:1121 ### SafeArguments: arg0 arg2 arg3 uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1311 ### SafeArguments: arg2 arg3 Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:316 NS_SecurityCompareURIs does some QIs. Error: AddRef/Release on nsISupports Location: nsCOMPtr<T>::~nsCOMPtr() [with T = nsIMutable] @ ff-dbg/dist/include/nsCOMPtr.h:404 ### SafeArguments: this Stack Trace: uint32 NS_EnsureSafeToReturn(nsIURI*, nsIURI**) @ netwerk/base/nsNetUtil.cpp:1470 ### SafeArguments: arg1 uint32 nsNullPrincipal::GetURI(nsIURI**) @ caps/nsNullPrincipal.cpp:112 ### SafeArguments: arg0 uint8 nsPrincipal::SubsumesInternal(nsIPrincipal*, uint32) @ caps/nsPrincipal.cpp:235 ### SafeArguments: arg1 uint8 mozilla::BasePrincipal::Subsumes(nsIPrincipal*, uint32) @ caps/BasePrincipal.cpp:340 ### SafeArguments: arg1 uint32 mozilla::BasePrincipal::Equals(nsIPrincipal*, uint8*) @ caps/BasePrincipal.cpp:348 ### SafeArguments: arg1 uint8 nsCSSValueTokenStream::operator==(nsCSSValueTokenStream*) const @ ff-dbg/dist/include/nsCSSValue.h:1780 uint8 nsCSSValue::operator==(nsCSSValue*) const @ layout/style/nsCSSValue.cpp:297 uint8 nsCSSValue::operator!=(nsCSSValue*) const @ ff-dbg/dist/include/nsCSSValue.h:616 uint8 nsCSSValue::Array::operator==(nsCSSValue::Array*) const @ ff-dbg/dist/include/nsCSSValue.h:1061 uint8 nsStyleContentData::operator==(nsStyleContentData*) const @ layout/style/nsStyleStruct.cpp:3698 bool nsTArray_Impl<E, Alloc>::operator==(const nsTArray_Impl<E, Allocator>&) const [with Allocator = nsTArrayInfallibleAllocator; E = nsStyleContentData; Alloc = nsTArrayInfallibleAllocator] @ ff-dbg/dist/include/nsTArray.h:1088 bool nsTArray_Impl<E, Alloc>::operator!=(const self_type&) const [with E = nsStyleContentData; Alloc = nsTArrayInfallibleAllocator; nsTArray_Impl<E, Alloc>::self_type = nsTArray_Impl<nsStyleContentData, nsTArrayInfallibleAllocator>] @ ff-dbg/dist/include/nsTArray.h:1098 uint32 nsStyleContent::CalcDifference(nsStyleContent*) const @ layout/style/nsStyleStruct.cpp:3751 uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; uint32_t = unsigned int] @ layout/style/nsStyleContext.cpp:1121 ### SafeArguments: arg0 arg2 arg3 uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1311 ### SafeArguments: arg2 arg3 Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:316 NS_EnsureSafeToReturn also has a QI. Error: Variable assignment nsCSSKeywords.cpp:int32_t gKeywordTableRefCount Location: void nsCSSKeywords::ReleaseTable() @ layout/style/nsCSSKeywords.cpp:47 Stack Trace: uint8 nsCSSProps::GetColorName(int32, nsCString*) @ layout/style/nsCSSProps.cpp:2538 ### SafeArguments: arg1 void nsCSSValue::AppendToString(int32, nsAString_internal*, uint32) const @ layout/style/nsCSSValue.cpp:1652 ### SafeArguments: this arg1 arg2 void nsCSSValue::AppendToString(int32, nsAString_internal*, uint32) const @ layout/style/nsCSSValue.cpp:1723 ### SafeArguments: arg1 arg2 uint8 mozilla::css::Declaration::AppendValueToString(int32, nsAString_internal*, uint32, uint8*) const @ layout/style/Declaration.cpp:271 ### SafeArguments: arg1 arg2 arg3 void mozilla::css::Declaration::AppendPropertyAndValueToString(int32, nsAString_internal*, nsAutoString*, uint8) const @ layout/style/Declaration.cpp:1629 ### SafeArguments: arg1 arg2 void mozilla::css::Declaration::ToString(nsAString_internal*) const @ layout/style/Declaration.cpp:1785 ### SafeArguments: arg0 void mozilla::DeclarationBlock::ToString(nsAString_internal*) const @ ff-dbg/dist/include/mozilla/DeclarationBlockInlines.h:58 ### SafeArguments: arg0 void nsAttrValue::ToString(nsAString_internal*) const @ dom/base/nsAttrValue.cpp:648 ### SafeArguments: arg0 uint8 nsAttrValue::Equals(nsIAtom*, uint32) const @ dom/base/nsAttrValue.cpp:1134 ServoBindings.cpp:uint8 AttrEquals(nsAttrValue*)::<lambda(const nsAttrValue*)> [with Implementor = const mozilla::dom::Element] @ layout/style/ServoBindings.cpp:480 ### SafeArguments: this ServoBindings.cpp:uint8 DoMatch(mozilla::dom::Element*, nsIAtom*, nsIAtom*, struct AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with Implementor = const mozilla::dom::Element]::<lambda(const class nsAttrValue*)>) [with Implementor = const mozilla::dom::Element; MatchFn = AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with Implementor = const mozilla::dom::Element]::<lambda(const nsAttrValue*)>] @ layout/style/ServoBindings.cpp:459 ServoBindings.cpp:uint8 AttrEquals(mozilla::dom::Element*, nsIAtom*, nsIAtom*, nsIAtom*, uint8) [with Implementor = const mozilla::dom::Element] @ layout/style/ServoBindings.cpp:482 Gecko_AttrEquals @ layout/style/ServoBindings.cpp:664 Location: nsCOMPtr<T>::~nsCOMPtr() [with T = nsIFile] @ ff-dbg/dist/include/nsCOMPtr.h:404 ### SafeArguments: this Stack Trace: uint32 nsLocalFile::Equals(nsIHashable*, uint8*) @ xpcom/io/nsLocalFileUnix.cpp:2192 ### SafeArguments: arg1 uint32 mozilla::net::nsStandardURL::EqualsInternal(nsIURI*, uint32, uint8*) @ netwerk/base/nsStandardURL.cpp:2265 ### SafeArguments: arg2 uint32 mozilla::net::nsStandardURL::Equals(nsIURI*, uint8*) @ netwerk/base/nsStandardURL.cpp:2175 ### SafeArguments: arg1 uint8 nsCSSValueTokenStream::operator==(nsCSSValueTokenStream*) const @ ff-dbg/dist/include/nsCSSValue.h:1776 uint8 nsCSSValue::operator==(nsCSSValue*) const @ layout/style/nsCSSValue.cpp:297 uint8 nsCSSValue::operator!=(nsCSSValue*) const @ ff-dbg/dist/include/nsCSSValue.h:616 void nsCSSValue::AppendSidesShorthandToString(int32*, nsCSSValue**, nsAString_internal*, uint32) @ layout/style/nsCSSValue.cpp:1183 ### SafeArguments: arg1 arg2 arg3 void nsCSSValue::AppendBasicShapeRadiusToString(int32*, nsCSSValue**, nsAString_internal*, uint32) @ layout/style/nsCSSValue.cpp:1214 ### SafeArguments: arg1 arg2 arg3 void nsCSSValue::AppendInsetToString(int32, nsAString_internal*, uint32) const @ layout/style/nsCSSValue.cpp:1253 ### SafeArguments: this arg1 arg2 void nsCSSValue::AppendToString(int32, nsAString_internal*, uint32) const @ layout/style/nsCSSValue.cpp:1456 ### SafeArguments: this arg1 arg2 void nsCSSValue::AppendToString(int32, nsAString_internal*, uint32) const @ layout/style/nsCSSValue.cpp:1723 ### SafeArguments: arg1 arg2 uint8 mozilla::css::Declaration::AppendValueToString(int32, nsAString_internal*, uint32, uint8*) const @ layout/style/Declaration.cpp:271 ### SafeArguments: arg1 arg2 arg3 void mozilla::css::Declaration::AppendPropertyAndValueToString(int32, nsAString_internal*, nsAutoString*, uint8) const @ layout/style/Declaration.cpp:1629 ### SafeArguments: arg1 arg2 void mozilla::css::Declaration::ToString(nsAString_internal*) const @ layout/style/Declaration.cpp:1785 ### SafeArguments: arg0 void mozilla::DeclarationBlock::ToString(nsAString_internal*) const @ ff-dbg/dist/include/mozilla/DeclarationBlockInlines.h:58 ### SafeArguments: arg0 void nsAttrValue::ToString(nsAString_internal*) const @ dom/base/nsAttrValue.cpp:648 ### SafeArguments: arg0 uint8 nsAttrValue::Equals(nsIAtom*, uint32) const @ dom/base/nsAttrValue.cpp:1134 ServoBindings.cpp:uint8 AttrEquals(nsAttrValue*)::<lambda(const nsAttrValue*)> [with Implementor = const mozilla::dom::Element] @ layout/style/ServoBindings.cpp:480 ### SafeArguments: this ServoBindings.cpp:uint8 DoMatch(mozilla::dom::Element*, nsIAtom*, nsIAtom*, struct AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with Implementor = const mozilla::dom::Element]::<lambda(const class nsAttrValue*)>) [with Implementor = const mozilla::dom::Element; MatchFn = AttrEquals(Implementor*, nsIAtom*, nsIAtom*, nsIAtom*, bool) [with Implementor = const mozilla::dom::Element]::<lambda(const nsAttrValue*)>] @ layout/style/ServoBindings.cpp:459 ServoBindings.cpp:uint8 AttrEquals(mozilla::dom::Element*, nsIAtom*, nsIAtom*, nsIAtom*, uint8) [with Implementor = const mozilla::dom::Element] @ layout/style/ServoBindings.cpp:482 Gecko_AttrEquals @ layout/style/ServoBindings.cpp:664 nsStandardURL::EqualsInternal has a QI

Filed bug 1343388 for most of the issues in comment 46, and bug 1343389 for the nsIPrincipal stuff.

Principal stuff is still underway, but everything else is taken care of. Can you do another run ignoring calls to nsIPrincipal::Equals and friends?

Sure, though it might not be for a couple of days (I'm out of town and don't have the right laptop with me). Also, in about a week I'll be going on a sailing trip and for three weeks to a month will only have email access. I won't be able to update my trees to do fresh runs of the analysis. If it would be better to avoid that gap then someone else could start running the analysis or we could integrate it into the hazard analysis and put it into automation.

Ok. Steve, can you get this analysis running on your setup?

Updated analysis file, sorry for the delay in getting to this. Here are the suspicious accesses I ran into while updating the analysis. Error: Dereference write hep Location: PL_HashTableRawLookup @ nsprpub/lib/ds/plhash.c:149 Stack Trace: PLHashEntry** nsFrameManagerBase::UndisplayedMap::GetEntryFor(nsIContent**) @ layout/base/nsFrameManager.cpp:729 ### SafeArguments: arg0 mozilla::UndisplayedNode* nsFrameManagerBase::UndisplayedMap::GetFirstNode(nsIContent*) @ layout/base/nsFrameManager.cpp:739 mozilla::UndisplayedNode* nsFrameManager::GetUndisplayedNodeInMapFor(nsFrameManagerBase::UndisplayedMap*, nsIContent*) @ layout/base/nsFrameManager.cpp:236 nsStyleContext* nsFrameManager::GetStyleContextInMap(nsFrameManagerBase::UndisplayedMap*, nsIContent*) @ layout/base/nsFrameManager.cpp:224 nsStyleContext* nsFrameManager::GetDisplayContentsStyleFor(nsIContent*) @ layout/base/nsFrameManager.h:135 Gecko_GetStyleContext @ layout/style/ServoBindings.cpp:299 Error: Field write nsStyleContext.mBits Location: void nsStyleContext::AddStyleBit(uint64*) @ layout/style/nsStyleContext.h:323 ### SafeArguments: arg0 Stack Trace: nsStyleEffects* nsStyleContext::DoGetStyleEffects() [with bool aComputeData = false] @ ff-dbg/dist/include/nsStyleStructList.h:151 nsStyleEffects* nsStyleContext::PeekStyleEffects() @ ff-dbg/dist/include/nsStyleStructList.h:151 uint32 nsStyleContext::CalcStyleDifferenceInternal(FakeStyleContext*, uint32, uint32*, uint32*) [with StyleContextLike = FakeStyleContext; uint32_t = unsigned int] @ ff-dbg/layout/style/nsStyleStructList.h:151 ### SafeArguments: arg0 arg2 arg3 uint32 nsStyleContext::CalcStyleDifference(ServoComputedValues*, uint32, uint32*, uint32*) @ layout/style/nsStyleContext.cpp:1328 ### SafeArguments: arg2 arg3 Gecko_CalcStyleDifference @ layout/style/ServoBindings.cpp:318 Error: Variable assignment int32_t nsXPLookAndFeel::sCachedColors [93] Location: uint32 nsXPLookAndFeel::GetColorImpl(uint8, uint8, uint32*) @ widget/nsXPLookAndFeel.cpp:826 ### SafeArguments: arg2 Stack Trace: uint32 mozilla::LookAndFeel::GetColor(uint8, uint8, uint32*) @ widget/nsXPLookAndFeel.cpp:915 ### SafeArguments: arg2 Gecko_GetLookAndFeelSystemColor @ layout/style/ServoBindings.cpp:495 Error: Field write nsCSSValue::Array.mRefCnt Location: void nsCSSValue::Array::AddRef() @ ff-dbg/dist/include/nsCSSValue.h:1088 Stack Trace: void nsStyleContentData::nsStyleContentData(nsStyleContentData*) @ layout/style/nsStyleStruct.cpp:3705 ### SafeArguments: this nsStyleContentData* nsStyleContentData::operator=(nsStyleContentData*) @ layout/style/nsStyleStruct.cpp:3720 ### SafeArguments: this Gecko_CopyStyleContentsFrom @ layout/style/ServoBindings.cpp:1036 ### SafeArguments: arg0

Jon - sorry, I know these changes probably won't mean much to you. Just make sure I'm not doing something *too* stupid. For the actual analysis, Brian wrote it and I'll review it, but I have a series of changes to make so that I can integrate it into the existing jobs.

I assume this is for (limited) handling of overloaded method names.

This should be a pure refactoring.

I originally thought that I'd be able to read from the existing callgraph.txt for the new analysis, but it turns out that it doesn't really make sense to do as a separate step for it. But record the information anyway, it makes things a little more understandable.

I will review this one. I mostly have already, but I should probably understand it a little better before landing it for real.

Another refactoring. Nothing much to see here.

Ok, this was the point of all the refactoring. callgraph.js now contains everything necessary for both the original analysis and the new one, so use it as the shared implementation.

Some real changes. Jon, the main thing is that we don't want to miss thread-unsafe functions, but we also don't want false positives.

Plumbing.

Comment on attachment 8851428 [details] [diff] [review] Refactor getCallees to reduce indent level Review of attachment 8851428 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/computeCallgraph.js @@ +202,5 @@ > + for (var name of functions) { > + if (name === null) { > + // Unknown set of call targets, meaning either a function pointer > + // call ("field call") or a virtual method that can be overridden > + // in extensions. Use the isVirtual property so that callers can You added a comment about the isVirtual property, but it doesn't seem to be used anywhere.

Comment on attachment 8851429 [details] [diff] [review] Record whether a call is a virtual function call vs some other field call Review of attachment 8851429 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/computeCallgraph.js @@ +186,5 @@ > if (suppressed) { > // Field call known to not GC; mark it as suppressed so direct > // invocations will be ignored > callees.push({'kind': "field", 'csu': csuName, 'field': fieldName, > + 'suppressed': true, isVirtual: true}); nit: there's quotes round some of the property names and not others.

Comment on attachment 8851433 [details] [diff] [review] Annotate away some false positives Review of attachment 8851433 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js @@ +90,5 @@ > function checkIndirectCall(entry, location, callee) > { > var name = entry.name; > > + if (callee.startsWith('malloc_table_t.')) Might be worth a comment saying what this is (replace malloc bridge?).

bholley - the request when reviewing this is to ask if you believe that ignoring the things here is valid. Most of them should be self-explanatory (and trivial). The ignoreContents changes are a little unfortunate, since they say that anything that eg nsString::StripChars does is safe as long as the string you're operating on is safe. It's a pretty wide exclusion, but hopefully reasonable.

MozReview-Commit-ID: 4ucyzmwzenJ

jonco, mostly asking for what you think of the URL I'm using. It goes to searchfox, and it'll give a bogus URL a fair percentage of the time, when functions are defined in header files that are accessed through <objdir>/dist/include. And it's questionable whether to use searchfox. hg.mozilla.org might be better. And whatever I use, it'd probably be better to embed the revision hash, though I don't have easy access to it. Anyway, it's all mostly irrelevant anyway. Just makes it a little easier to work through these.

Make specific messaging for the two types of hazard failures. I also added a section to the linked wiki page describing these new hazards. I probably ought to do this via the expectation files that I'm no longer using. Future work.

MozReview-Commit-ID: 3WbGktCxymN

Comment on attachment 8857679 [details] [diff] [review] Rewrite locations to URLs Review of attachment 8857679 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js @@ +637,5 @@ > +else if (m = scriptArgs[0].match(/--strip-prefix=(.*)/)) > + removePrefix = m[1]; > +if (removePrefix && !removePrefix.endsWith("/")) > + removePrefix += '/'; > +var addPrefix = 'https://searchfox.org/mozilla-central/source/'; Seems fine to link to searchfox if you find that useful. It would be better if we had revision information but no big deal. @@ +771,5 @@ > dumpError(entry, location, "Unknown assignment " + JSON.stringify(lhs)); > } > > +function get_location(rawLocation) { > + const filename = rawLocation.CacheString.replace(removePrefix, ''); I guess this will remove from anywhere in the string, not just the front.

Comment on attachment 8857695 [details] [diff] [review] Unexpected heap write hazards should cause the hazard job to fail Review of attachment 8857695 [details] [diff] [review]: ----------------------------------------------------------------- ::: taskcluster/scripts/builder/hazard-analysis.sh @@ +133,5 @@ > set +e > NUM_HAZARDS=$(grep -c 'Function.*has unrooted.*live across GC call' "$1"/rootingHazards.txt) > NUM_UNSAFE=$(grep -c '^Function.*takes unsafe address of unrooted' "$1"/refs.txt) > NUM_UNNECESSARY=$(grep -c '^Function.* has unnecessary root' "$1"/unnecessary.txt) > + NUM_WRITE_HAZARDS=$(perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!') Should you be passing a filename here? Also we already depend on perl? I guess we probably do. @@ +147,5 @@ > + echo "TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit \"Inspect Task\" link for hazard details" > + exit 1 > + fi > + > + NUM_ALLOWED_WRITE_HAZARDS=7 It looks like the allowed hazards information is already present in the log file processed above so it would be better to get if from there if we can.

Comment on attachment 8857674 [details] [diff] [review] Straightforward heap write annotations Review of attachment 8857674 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for making this happen! The consensus in the Taipei meetup is that we desperately need to get this analysis turned on. :-) ::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js @@ +144,5 @@ > [/^Gecko_/, null, "nsStyleImageLayers"], > [/^Gecko_/, null, /FontFamilyList/], > + [/^Gecko_/, null, /nsStyleDisplay/], > + [/^Gecko_/, null, /nsStyleFont/], > + [/^Gecko_/, null, /nsStyleImage/], I'm a bit wary whitelisting these types rather than the specific functions that use them as out-parameters. Is the issue that there are just too many of them? Naively, I'd prefer these all to go with the list below, but I don't care so much if we do the attribute thing. @@ +149,2 @@ > > // Various Servo binding out parameters. This is a mess and there needs Note that out param isn't quite right for e.g. Gecko_SetNodeFlags - it's not an outparam, it's just a param that points to mutable heap memory. @@ +149,4 @@ > > // Various Servo binding out parameters. This is a mess and there needs > // to be a way to indicate which params are out parameters, either using > // an attribute or a naming convention. Yes please! I guess an attribute is probably a bit nicer. Note that some of the functions in this list don't exist anymore. But again, probably ok if we're about to switch to attributes. @@ +351,5 @@ > / EmptyString\(\)/, > /nsCSSProps::LookupPropertyValue/, > /nsCSSProps::ValueToKeyword/, > /nsCSSKeywords::GetStringValue/, > + /nsCSSRuleProcessor::HasSystemMetric/, Hm, this doesn't belong with the others - it calls into InitSystemMetrics, which doesn't look threadsafe to me! Please file a CSS bug with "stylo: " in the prefix and NI Manishearth.

MozReview-Commit-ID: 8XvBhvxNyCE

(In reply to Jon Coppeard (:jonco) from comment #70) > @@ +133,5 @@ > > set +e > > NUM_HAZARDS=$(grep -c 'Function.*has unrooted.*live across GC call' "$1"/rootingHazards.txt) > > NUM_UNSAFE=$(grep -c '^Function.*takes unsafe address of unrooted' "$1"/refs.txt) > > NUM_UNNECESSARY=$(grep -c '^Function.* has unnecessary root' "$1"/unnecessary.txt) > > + NUM_WRITE_HAZARDS=$(perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!') > > Should you be passing a filename here? Thank you for finding the error before I even looked at my failed try push! > Also we already depend on perl? I guess we probably do. I'm not sure. I know it's available. I could use sed instead, but I'd have to figure out how. > @@ +147,5 @@ > > + echo "TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit \"Inspect Task\" link for hazard details" > > + exit 1 > > + fi > > + > > + NUM_ALLOWED_WRITE_HAZARDS=7 > > It looks like the allowed hazards information is already present in the log > file processed above so it would be better to get if from there if we can. It's different. I think bhackett used the maximum of 100 in the script to keep the analysis from spewing out too much stuff. ("Only report up to 100 errors because reporting any more than that is not going to be useful.") I could probably remove it now, and at any rate, the hardcoded value there is hardcoded much deeper than in this script. This script is the one concerned with deciding whether to turn the job red.

Comment on attachment 8857676 [details] [diff] [review] Use parameter names when reporting heap write hazards, when available Ugh. The parameter name reporting was a trivial thing that doesn't change the algorithm at all, just makes the output a little nicer. But I'm going to clear review here, because it looks like I accidentally swept up some other changes into this patch. :-(

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #71) > ::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js > @@ +144,5 @@ > > [/^Gecko_/, null, "nsStyleImageLayers"], > > [/^Gecko_/, null, /FontFamilyList/], > > + [/^Gecko_/, null, /nsStyleDisplay/], > > + [/^Gecko_/, null, /nsStyleFont/], > > + [/^Gecko_/, null, /nsStyleImage/], > > I'm a bit wary whitelisting these types rather than the specific functions > that use them as out-parameters. Is the issue that there are just too many > of them? Naively, I'd prefer these all to go with the list below, but I > don't care so much if we do the attribute thing. No. I think I misunderstood something you had told me earlier. I will do this per-function. > @@ +149,2 @@ > > > > // Various Servo binding out parameters. This is a mess and there needs > > Note that out param isn't quite right for e.g. Gecko_SetNodeFlags - it's not > an outparam, it's just a param that points to mutable heap memory. Good point. I wrote the comment when I was less familiar with what's going on. I will reword. But that brings up a question -- what is the right term for this? You say "mutable". I guess that's in reference to Rust's mutable thing? Because it's not C++'s mutable, nor is it "capable of being mutated" since that would cover shared writable state that is accessible while holding a lock. Maybe "owned"? Or "thread owned"? Or "thread local"? "Thread mutable"? (Maybe that last is what you meant by mutable.) > @@ +149,4 @@ > > > > // Various Servo binding out parameters. This is a mess and there needs > > // to be a way to indicate which params are out parameters, either using > > // an attribute or a naming convention. > > Yes please! I guess an attribute is probably a bit nicer. I can't remember what locations sixgill is currently capable of extracting annotations from. But I don't think it can do parameters yet. I need to take a look at that and see how doable it is. > Note that some of the functions in this list don't exist anymore. But again, > probably ok if we're about to switch to attributes. Yeah, I could have it automatically report which annotations were never used, to trim down the lists. But for this, I think I'd rather postpone doing anything. > @@ +351,5 @@ > > / EmptyString\(\)/, > > /nsCSSProps::LookupPropertyValue/, > > /nsCSSProps::ValueToKeyword/, > > /nsCSSKeywords::GetStringValue/, > > + /nsCSSRuleProcessor::HasSystemMetric/, > > Hm, this doesn't belong with the others - it calls into InitSystemMetrics, > which doesn't look threadsafe to me! Please file a CSS bug with "stylo: " in > the prefix and NI Manishearth. Whoops, I messed that one up. I forget what it was complaining about... hm. I just removed it and it had no effect. Oh, that's because it's being masked by an earlier reason for a hazard in Gecko_MatchStringArgPseudo... but that's a false positive. Reapplying all the other patches makes InitSystemMetrics reappear. Ok, for some reason I was assuming that InitSystemMetrics would be called on the main thread before any other threads saw it, but I see no reason for thinking that now. https://bugzilla.mozilla.org/show_bug.cgi?id=1356305

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #71) > @@ +149,2 @@ > > > > // Various Servo binding out parameters. This is a mess and there needs > > Note that out param isn't quite right for e.g. Gecko_SetNodeFlags - it's not > an outparam, it's just a param that points to mutable heap memory. Looking at this, is it fair to say that any RawGeckoNodeBorrowed is safe? (I'm just going by the name.)

Comment on attachment 8858162 [details] [diff] [review] Handle templatized C1/C4 constructors I'm not sure there's really a point in reviewing this, but I guess I'll request review to cover my a**.

Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e67e7bedfd3 Rename addEntry, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4a8e13b6f8 Tag fields containing functions with arg count, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/1eecc3f4c000 Refactor getCallees to reduce indent level, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/124730f4f4cf Record whether a call is a virtual function call vs some other field call, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9e8f827356 Import untouched version of heap write analysis, r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/6eed05324994 Move code that analyzes bodies to generate info about the callgraph into callgraph.js, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/ba26b26cf859 Switch analyzeHeapWrites to callgraph.js, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/625e5f04a883 Annotate away some false positives, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/73933ce1b503 Add analyzeHeapWrites.js to the hazard jobs, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/b344ebd5d16e Use parameter names when reporting heap write hazards, when available, r=me https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d95fa8ba3b Straightforward heap write annotations, r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/80f0f5f07dcb Rewrite locations to URLs, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/c41f4231fcb2 trivial changes to the analysis code. r=woof! https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb095331f9f Allow AsAString to propagate safety, r=bhackett https://hg.mozilla.org/integration/mozilla-inbound/rev/3b3eb89732eb Safe UniquePtr content can be considered safe, r=bhackett https://hg.mozilla.org/integration/mozilla-inbound/rev/5d394e8bee5b Unexpected heap write hazards should cause the hazard job to fail, r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/27af81fdade2 Handle templatized C1/C4 constructors, r=bhackett

(In reply to Steve Fink [:sfink] [:s:] from comment #75) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #71) > > ::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js > > @@ +144,5 @@ > > > [/^Gecko_/, null, "nsStyleImageLayers"], > > > [/^Gecko_/, null, /FontFamilyList/], > > > + [/^Gecko_/, null, /nsStyleDisplay/], > > > + [/^Gecko_/, null, /nsStyleFont/], > > > + [/^Gecko_/, null, /nsStyleImage/], > > > > I'm a bit wary whitelisting these types rather than the specific functions > > that use them as out-parameters. Is the issue that there are just too many > > of them? Naively, I'd prefer these all to go with the list below, but I > > don't care so much if we do the attribute thing. > > No. I think I misunderstood something you had told me earlier. I will do > this per-function. > > > @@ +149,2 @@ > > > > > > // Various Servo binding out parameters. This is a mess and there needs > > > > Note that out param isn't quite right for e.g. Gecko_SetNodeFlags - it's not > > an outparam, it's just a param that points to mutable heap memory. > > Good point. I wrote the comment when I was less familiar with what's going > on. I will reword. > > But that brings up a question -- what is the right term for this? You say > "mutable". I guess that's in reference to Rust's mutable thing? Because it's > not C++'s mutable, nor is it "capable of being mutated" since that would > cover shared writable state that is accessible while holding a lock. > > Maybe "owned"? Or "thread owned"? Or "thread local"? "Thread mutable"? > (Maybe that last is what you meant by mutable.) "Mutable by this thread" is what I meant, which seems clear enough from context. But I think "owned" works fine too.

Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13941e8c1b9d Bump number of allowed write hazards r=bholley CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/0e67e7bedfd3 https://hg.mozilla.org/mozilla-central/rev/9b4a8e13b6f8 https://hg.mozilla.org/mozilla-central/rev/1eecc3f4c000 https://hg.mozilla.org/mozilla-central/rev/124730f4f4cf https://hg.mozilla.org/mozilla-central/rev/6b9e8f827356 https://hg.mozilla.org/mozilla-central/rev/6eed05324994 https://hg.mozilla.org/mozilla-central/rev/ba26b26cf859 https://hg.mozilla.org/mozilla-central/rev/625e5f04a883 https://hg.mozilla.org/mozilla-central/rev/73933ce1b503 https://hg.mozilla.org/mozilla-central/rev/b344ebd5d16e https://hg.mozilla.org/mozilla-central/rev/f2d95fa8ba3b https://hg.mozilla.org/mozilla-central/rev/80f0f5f07dcb https://hg.mozilla.org/mozilla-central/rev/c41f4231fcb2 https://hg.mozilla.org/mozilla-central/rev/aeb095331f9f https://hg.mozilla.org/mozilla-central/rev/3b3eb89732eb https://hg.mozilla.org/mozilla-central/rev/5d394e8bee5b https://hg.mozilla.org/mozilla-central/rev/27af81fdade2 https://hg.mozilla.org/mozilla-central/rev/13941e8c1b9d