Crash [@ js::TraceLoggerEvent::TraceLoggerEvent]

RESOLVED FIXED in Firefox 52

Status

()

--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gkw, Assigned: h4writer)

Tracking

(Blocks: 1 bug, {crash, jsbugmon, testcase})

Trunk
mozilla52
x86_64
Mac OS X
crash, jsbugmon, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox51 affected, firefox52 fixed)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision 955840bfd3c2 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

function h1(f) {
    for (var j = 0; j < 99; ++j) {
        try {
            f();
        } catch (e) {
            "" + e;
        }
    }
}
function m(x) {
    Function(x)()
}
m("\
    function g1() {\
        du.setupTraceLoggerScriptCalls();\
        du.startTraceLogger();\
    }\
    function g2() {\
        g1();\
    }\
    var du = Debugger();\
    g2();\
");
m("\
    h1(function() {});\
    function h2() {\
        return function ()(\
            function h3() {\
                h3();\
            }()\
        );\
    }\
    h1(h2());\
")


Backtrace:

0   js-dbg-64-dm-clang-darwin-955840bfd3c2	0x000000010852f33c js::TraceLoggerEvent::TraceLoggerEvent(js::TraceLoggerThread*, TraceLoggerTextId, JSScript*) + 76 (TraceLogging.h:150)
1   js-dbg-64-dm-clang-darwin-955840bfd3c2	0x0000000108bfad49 Interpret(JSContext*, js::RunState&) + 34921 (Interpreter.cpp:2989)
2   js-dbg-64-dm-clang-darwin-955840bfd3c2	0x0000000108bf22e4 js::RunScript(JSContext*, js::RunState&) + 452 (Interpreter.cpp:404)
3   js-dbg-64-dm-clang-darwin-955840bfd3c2	0x0000000108c0507f js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 511 (Interpreter.cpp:685)
/snip

For detailed crash information, see attachment.
(Reporter)

Comment 1

2 years ago
Created attachment 8797365 [details]
Detailed Crash Information
(Reporter)

Comment 2

2 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/7a942a270777
user:        Hannes Verschore
date:        Tue Jun 21 13:52:11 2016 +0200
summary:     Bug 1280648 - Tracelogger: Don't cache based on pointers to movable gc things, r=bbouvier

Hannes, is bug 1280648 a likely regressor?

Note that this may take 10-15 seconds to crash. Also, it would be good to backport this at least to mozilla-aurora, because it is hard to get a reliable testcase.
Blocks: 1280648
Flags: needinfo?(hv1989)
(Assignee)

Comment 3

2 years ago
Created attachment 8802898 [details] [diff] [review]
Patch

This fixes the issue. The AutoTraceLog could overflow the memory and as a result we would check every Payload and remove if the "use count" was 0. Since we just created it, it is still zero. We only increase it in the caller. 
Fix is to temporarily set it to 0. Making sure we survive the AutoTraceLog and set it to 0, just before returning.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8802898 - Flags: review?(bbouvier)
Comment on attachment 8802898 [details] [diff] [review]
Patch

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

Sorry, cancelling review: I'd like to see a simpler (if possible) test case triggering this, included in the patch. I don't understand the initial issue (this AutoTraceLog uses the uint32_t ctor, not the Event ctor, so there's no use count involved here?), so I don't understand the fix either. The usage of ScopeExit could probably be made simpler.

::: js/src/vm/TraceLogging.cpp
@@ +384,5 @@
>          return nullptr;
>      }
>  
> +    // Temporarily mark the payload as used. To make sure it doesn't get GC'ed.
> +    payload->use();

I think this needs its own ScopeExit with a payload->release() (if one of the functions are failing too in the meanwhile, like the call to pointerMap.add)

@@ +396,5 @@
>          return nullptr;
>  
> +    // Emit the end of the internal time and remove the temporary mark.
> +    guardInternalStopEvent.release();
> +    stopEvent(TraceLogger_Internal);

The comment at the top of MakeScopeExit seems to say that the functor is called at the end of scope anyways; in this case, we could just remove the last two lines, right?

@@ +477,5 @@
>  
> +    // Emit the end of the internal time and remove the temporary mark.
> +    guardInternalStopEvent.release();
> +    stopEvent(TraceLogger_Internal);
> +    payload->release();

Same comments apply.
Attachment #8802898 - Flags: review?(bbouvier)
(Assignee)

Comment 5

2 years ago
Created attachment 8803311 [details] [diff] [review]
Patch

After discussion on irc we think this is better? Like said on IRC adding a testcase will be hard, since it needs to be timed and consume the whole buffer.
Attachment #8803311 - Flags: review?(bbouvier)
(Assignee)

Comment 6

2 years ago
Created attachment 8803312 [details] [diff] [review]
Patch

With better hgrc to include more context
Attachment #8802898 - Attachment is obsolete: true
Attachment #8803311 - Attachment is obsolete: true
Attachment #8803311 - Flags: review?(bbouvier)
Attachment #8803312 - Flags: review?(bbouvier)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8803312 [details] [diff] [review]
Patch

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

::: js/src/vm/TraceLogging.cpp
@@ +383,5 @@
>          return nullptr;
>      }
>  
>      if (!textIdPayloads.putNew(textId, payload)) {
>          js_delete(payload);

add
payload = nullptr;
Comment on attachment 8803312 [details] [diff] [review]
Patch

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

Thanks for the explanation on IRC (and for including more context in your patches).

::: js/src/vm/TraceLogging.cpp
@@ +383,5 @@
>          return nullptr;
>      }
>  
>      if (!textIdPayloads.putNew(textId, payload)) {
>          js_delete(payload);

Alternatively: in the guard, if (payload && payload->uses())

This way, you could even use a UniquePtr<TraceLoggerEventPayload> and remove these ugly calls to js_delete ;)
Attachment #8803312 - Flags: review?(bbouvier) → review+

Comment 9

2 years ago
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/062db36d2f0b
TraceLogging - Make sure the payload is marked as used during creation, r=bbouvier

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/062db36d2f0b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
status-firefox50: --- → affected
status-firefox51: --- → affected
You need to log in before you can comment on or make changes to this bug.