1294915 - (stylo-static-analysis) stylo: Explore more robust checking against FFI race conditions

Reporter

Description

•

8 years ago

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?

Bobby Holley (:bholley)

Reporter

•

8 years ago

(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?

Flags: needinfo?(sphink)

Steve Fink [:sfink] [:s:]

Assignee

Comment 4

•

8 years ago

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.

Flags: needinfo?(sphink)

(no longer active)

Comment 5

•

8 years ago

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.

Flags: needinfo?(bobbyholley)

Bobby Holley (:bholley)

Reporter

Comment 6

•

8 years ago

<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

Flags: needinfo?(bobbyholley) → needinfo?(ehsan)

Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]

Updated

•

8 years ago

Updated

•

8 years ago

Priority: -- → P3

(no longer active)

Comment 7

•

8 years ago

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.

Flags: needinfo?(ehsan)

Bobby Holley (:bholley)

Reporter

•

8 years ago

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.

Flags: needinfo?(sphink) → needinfo?(bhackett1024)

Brian Hackett [Laid off!]

Comment 12

•

8 years ago

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.

Flags: needinfo?(bhackett1024)

Nika Layzell [:nika] (ni? for response)

Comment 13

•

8 years ago

(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.

Bobby Holley (:bholley)

Reporter

Comment 14

•

8 years ago

(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

Flags: needinfo?(bhackett1024)

Brian Hackett [Laid off!]

Comment 15

•

8 years ago

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

Bobby Holley (:bholley)

Reporter

Comment 16

•

8 years ago

(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. :-)

Chris Peterson [:cpeterson]

Updated

•

8 years ago

Assignee: nobody → bhackett1024

Blocks: stylo-nightly
No longer blocks: stylo

Bobby Holley (:bholley)

Reporter

Comment 17

•

8 years ago

(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).

Brian Hackett [Laid off!]

Comment 18

•

8 years ago

Attached file WIP (obsolete) — Details

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.

Flags: needinfo?(bhackett1024)

Brian Hackett [Laid off!]

Comment 19

•

8 years ago

Attached file callgraphUtility.js (obsolete) — Details

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.

Brian Hackett [Laid off!]

Updated

•

8 years ago

•

8 years ago

(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?

Brian Hackett [Laid off!]

•

8 years ago

(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.

Bobby Holley (:bholley)

Reporter

Comment 29

•

8 years ago

(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()))?

Brian Hackett [Laid off!]

Comment 30

•

8 years ago

(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.

Steve Fink [:sfink] [:s:]

Assignee

Comment 31

•

8 years ago

(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).

Brian Hackett [Laid off!]

Comment 32

•

8 years ago

Attached file analyzeHeapWrites.js (obsolete) — Details

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::Release(U*) [with U = mozilla::CounterStyle] @ ff-dbg/dist/include/mozilla/RefPtr.h:40 static void RefPtr<T>::ConstRemovingRefPtrTraits::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?

Attachment #8829478 - Attachment is obsolete: true

Brian Hackett [Laid off!]

Comment 33

•

8 years ago

Attached file callgraphUtility.js — Details

This is slightly different from the earlier version.

Attachment #8829479 - Attachment is obsolete: true

Attachment #8829492 - Attachment is obsolete: true

Emilio Cobos Álvarez (:emilio)

•

8 years ago

No longer blocks: 1335305

Comment 40

•

8 years ago

(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.

Bobby Holley (:bholley)

Reporter

Comment 41

•

8 years ago

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?

Flags: needinfo?(bhackett1024)

Brian Hackett [Laid off!]

Comment 42

•

8 years ago

(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.

Brian Hackett [Laid off!]

Comment 43

•

8 years ago

Attached file analyzeHeapWrites.js (obsolete) — Details

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

Attachment #8830396 - Attachment is obsolete: true

Flags: needinfo?(bhackett1024)

Bobby Holley (:bholley)

Reporter

Updated

•

8 years ago

Depends on: 1340333

Bobby Holley (:bholley)

Reporter

Updated

•

8 years ago

Depends on: 1340339

Bobby Holley (:bholley)

Reporter

Updated

•

8 years ago

Depends on: 1340340

Bobby Holley (:bholley)

Reporter

Updated

•

8 years ago

Depends on: 1340341

Bobby Holley (:bholley)

Reporter

•

8 years ago

Attached file analyzeHeapWrites.js (obsolete) — Details

•

8 years ago

Attached file analyzeHeapWrites.js — Details

•

8 years ago

Attached patch Rename addEntry — Details — Splinter Review

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.

Attachment #8851426 - Flags: review?(jcoppeard)

Steve Fink [:sfink] [:s:]

Assignee

•

8 years ago

Attached patch Add analyzeHeapWrites.js to the hazard jobs — Details — Splinter Review

Plumbing.

Attachment #8851434 - Flags: review?(jcoppeard)

Jon Coppeard (:jonco)

Updated

•

8 years ago

Attachment #8851426 - Flags: review?(jcoppeard) → review+

Jon Coppeard (:jonco)

•

8 years ago

Attachment #8851431 - Flags: review?(jcoppeard) → review+

Jon Coppeard (:jonco)

•

8 years ago

Depends on: 1354358

Emilio Cobos Álvarez (:emilio)

Updated

•

8 years ago

Depends on: 1354966

•

8 years ago

Attached patch Allow AsAString to propagate safety — Details — Splinter Review

MozReview-Commit-ID: 3WbGktCxymN

Attachment #8857715 - Flags: review?(bhackett1024)

Brian Hackett [Laid off!]

Updated

•

8 years ago

Attachment #8857715 - Flags: review?(bhackett1024) → review+

Jon Coppeard (:jonco)

•

8 years ago

Depends on: 1356275

Steve Fink [:sfink] [:s:]

Assignee

Comment 72

•

8 years ago

Attached patch Safe UniquePtr content can be considered safe — Details — Splinter Review

MozReview-Commit-ID: 8XvBhvxNyCE

Attachment #8858005 - Flags: review?(bhackett1024)

Steve Fink [:sfink] [:s:]

Assignee

Comment 73

•

8 years ago

(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 <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.

Flags: needinfo?(sphink)

Steve Fink [:sfink] [:s:]

Assignee

Updated

•

8 years ago

Attachment #8851430 - Flags: review?(sphink) → review+

Steve Fink [:sfink] [:s:]

Assignee

Comment 74

•

8 years ago

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. :-(

Attachment #8857676 - Flags: review?(sphink)

Steve Fink [:sfink] [:s:]

Assignee

Updated

•

8 years ago

Depends on: 1356305

Steve Fink [:sfink] [:s:]

Assignee

Comment 75

•

8 years ago

(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

Steve Fink [:sfink] [:s:]

Assignee

Comment 76

•

8 years ago

(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.)

Brian Hackett [Laid off!]

Updated

•

8 years ago

Attachment #8858005 - Flags: review?(bhackett1024) → review+

Steve Fink [:sfink] [:s:]

Assignee

Comment 77

•

8 years ago

Attached patch Handle templatized C1/C4 constructors — Details — Splinter Review

Attachment #8858162 - Flags: review?(sphink)

Steve Fink [:sfink] [:s:]

Assignee

Comment 78

•

8 years ago

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**.

Attachment #8858162 - Flags: review?(sphink) → review?(bhackett1024)

Brian Hackett [Laid off!]

Updated

•

8 years ago

Attachment #8858162 - Flags: review?(bhackett1024) → review+

Pulsebot

Comment 79

•

8 years ago

Bobby Holley (:bholley)

Reporter

Comment 80

•

8 years ago

(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.

Bobby Holley (:bholley)

Reporter

•

8 years ago

Depends on: 1356276

Iris Hsiao [:ihsiao]

Comment 82

•

8 years ago

bugherder

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

WIP 8 years ago Brian Hackett [Laid off!] 5.76 KB, text/plain		Details
callgraphUtility.js 8 years ago Brian Hackett [Laid off!] 7.23 KB, application/javascript		Details
analyzeHeapWrites.js 8 years ago Brian Hackett [Laid off!] 14.62 KB, application/javascript		Details
callgraphUtility.js 8 years ago Brian Hackett [Laid off!] 7.66 KB, application/javascript		Details
visited functions 8 years ago Brian Hackett [Laid off!] 39.17 KB, text/plain		Details
analyzeHeapWrites.js 8 years ago Brian Hackett [Laid off!] 28.95 KB, application/javascript		Details
callgraphUtility.js 8 years ago Brian Hackett [Laid off!] 7.68 KB, application/javascript		Details
analyzeHeapWrites.js 8 years ago Brian Hackett [Laid off!] 30.54 KB, application/javascript		Details
analyzeHeapWrites.js 8 years ago Brian Hackett [Laid off!] 32.95 KB, application/javascript		Details
analyzeHeapWrites.js 8 years ago Brian Hackett [Laid off!] 33.76 KB, application/javascript		Details
Rename addEntry 8 years ago Steve Fink [:sfink] [:s:] 2.23 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Tag fields containing functions with arg count 8 years ago Steve Fink [:sfink] [:s:] 3.74 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Refactor getCallees to reduce indent level 8 years ago Steve Fink [:sfink] [:s:] 5.88 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Record whether a call is a virtual function call vs some other field call 8 years ago Steve Fink [:sfink] [:s:] 7.24 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Import untouched version of heap write analysis 8 years ago Steve Fink [:sfink] [:s:] 43.44 KB, patch	sfink : review+	Details \| Diff \| Splinter Review
Move code that analyzes bodies to generate info about the callgraph into callgraph.js 8 years ago Steve Fink [:sfink] [:s:] 17.78 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Switch analyzeHeapWrites to callgraph.js 8 years ago Steve Fink [:sfink] [:s:] 11.42 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Annotate away some false positives 8 years ago Steve Fink [:sfink] [:s:] 4.91 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Add analyzeHeapWrites.js to the hazard jobs 8 years ago Steve Fink [:sfink] [:s:] 3.17 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Straightforward heap write annotations 8 years ago Steve Fink [:sfink] [:s:] 9.37 KB, patch	bholley : review+	Details \| Diff \| Splinter Review
Use parameter names when reporting heap write hazards, when available 8 years ago Steve Fink [:sfink] [:s:] 8.50 KB, patch		Details \| Diff \| Splinter Review
Rewrite locations to URLs 8 years ago Steve Fink [:sfink] [:s:] 3.22 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Unexpected heap write hazards should cause the hazard job to fail 8 years ago Steve Fink [:sfink] [:s:] 2.57 KB, patch	jonco : review+	Details \| Diff \| Splinter Review
Allow AsAString to propagate safety 8 years ago Steve Fink [:sfink] [:s:] 1.67 KB, patch	bhackett1024 : review+	Details \| Diff \| Splinter Review
Safe UniquePtr content can be considered safe 8 years ago Steve Fink [:sfink] [:s:] 1.29 KB, patch	bhackett1024 : review+	Details \| Diff \| Splinter Review
Handle templatized C1/C4 constructors 8 years ago Steve Fink [:sfink] [:s:] 2.45 KB, patch	bhackett1024 : review+	Details \| Diff \| Splinter Review