Make the analysis consider whether a virtual call can GC when all targets are known

RESOLVED FIXED in mozilla28



5 years ago
5 years ago


(Reporter: sfink, Assigned: sfink)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [qa-])


(1 attachment)



5 years ago

Function 'js::ScriptSourceObject* js::ScriptSourceObject::create(js::ExclusiveContext*, js::ScriptSource*, JS::ReadOnlyCompileOptions*)' has unrooted '__temp_11' of type 'js::ScriptSourceObject*' live across GC call JS::ReadOnlyCompileOptions.element at js/src/jsscript.cpp:1009
    js/src/jsscript.cpp:1009: Call(21,22, __temp_13 := options*.element*())
    js/src/jsscript.cpp:1009: Call(22,23, __temp_12 := ObjectOrNullValue(__temp_13*))
    js/src/jsscript.cpp:1009: Call(23,24, __temp_11*.field:0.field:0.initSlot(1,__temp_12))

The problem is that ReadOnlyCompileOptions::element() is a virtual function call. The implementations are trivial and certainly cannot GC, and the analysis knows this when constructing the callgraph. But when looking for hazards, the callgraph is not involved for FieldCalls, so it assumes that any non-annotated FieldCall can gc.

Comment 1

5 years ago
Created attachment 8343940 [details] [diff] [review]
Make the analysis consider whether a virtual call can GC when all targets are known

When determining all targets of a virtual function call, record those targets in callgraph.txt. Then when computing gcFunctions, check whether any of them can GC and if not, add the FieldCall into suppressedFunctions. It turns out that suppressedFunctions *already* contained field calls, so this turns out to be straightforward. Note that any virtual function call that cannot be resolved to a known set of target callees will remain the same as it is now, and so will be assumed to GC.

This brings the current shell hazard count down to zero (!).
Attachment #8343940 - Flags: review?(bhackett1024)
Comment on attachment 8343940 [details] [diff] [review]
Make the analysis consider whether a virtual call can GC when all targets are known

Review of attachment 8343940 [details] [diff] [review]:

::: js/src/devtools/rootAnalysis/analyzeRoots.js
@@ +237,5 @@
>          var csuName = field.FieldCSU.Type.Name;
>          var fullFieldName = csuName + "." + field.Name[0];
>          if (fieldCallCannotGC(csuName, fullFieldName))
>              return null;
> +        //printErr(fullFieldName + ": " + ((fullFieldName in suppressedFunctions) ? "suppressed" : "cangc"));

rm debugging code

::: js/src/devtools/rootAnalysis/computeCallgraph.js
@@ +151,5 @@
> +                // calls to all possible resolved types, but also record edges
> +                // from this field call to each final callee. When the analysis
> +                // is checking whether an edge can GC and it sees an unrooted
> +                // pointer held live across this field call, it will know
> +                // whether any of the direct callers can GC or not.


::: js/src/vm/StructuredClone.cpp
@@ +395,5 @@
> +        if (tag == SCTAG_TRANSFER_MAP_ARRAY_BUFFER) {
> +            if (ownership == JS::SCTAG_TMO_ALLOC_DATA)
> +                js_free(content);
> +        } else if (cb && cb->freeTransfer) {
> +            cb->freeTransfer(buffer, tag, JS::TransferableOwnership(ownership), content, cbClosure);

This change looks unrelated?
Attachment #8343940 - Flags: review?(bhackett1024) → review+
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28


5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.