1475381 - Crash in js::frontend::TokenStreamSpecific<T>::identifierName

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-6c136416-7937-48b2-b097-0a6f30180712.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll js::frontend::TokenStreamSpecific<char16_t, js::frontend::ParserAnyCharsAccess<js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t> > >::identifierName js/src/frontend/TokenStream.cpp:1484
1 xul.dll js::frontend::TokenStreamSpecific<char16_t, js::frontend::ParserAnyCharsAccess<js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t> > >::getTokenInternal js/src/frontend/TokenStream.cpp:1902
2 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::memberExpr js/src/frontend/Parser.cpp:8745
3 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::unaryExpr js/src/frontend/Parser.cpp:8560
4 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::assignExpr js/src/frontend/Parser.cpp:8264
5 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::expr js/src/frontend/Parser.cpp:7930
6 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::expressionStatement js/src/frontend/Parser.cpp:6132
7 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementListItem js/src/frontend/Parser.cpp:7768
8 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementList js/src/frontend/Parser.cpp:4253
9 xul.dll js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::functionBody js/src/frontend/Parser.cpp:2762

=============================================================

this crash signature is newly showing up in firefox 61 and subsequent builds.

Comment 1

[Nicolas B. Pierron [:nbp]]

Waldo, is this issue actionable?

Comment 2

[Jeff Walden [:Waldo]]

The top function -- TokenStreamSpecific::identifierName -- is newly extracted from TokenStreamSpecific::getTokenInternal where it had resided for quite some time.  The mere presence of crashes inside this function doesn't necessarily prove anything -- they could have just transferred from the one function to the other.

Setting that aside, the crash location here has been further altered on trunk -- including so that we know which arm of the prior if-else was followed.  bp-9a181ac1-4a44-446f-a46d-3f6650180708 looks like a better report to investigate, and it suggests that the crash is something about creating an atom from source text.

Past that, I'm not immediately sure what to investigate next about this, or what can be investigated next.  Will think about it more before clearing the needinfo.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

waldo: any results from thinking?

Comment 4

[Jeff Walden [:Waldo]]

Good news, maybe?

As I look at it now, there are no crashes in 63 or later.  The particular change I mentioned in comment 2, that would help to indicate precisely which arm was going awry, was https://hg.mozilla.org/mozilla-central/rev/b64aec5cf563 which that page sez is 63.0a1.  So I *think* what's probably happened (unless I fixed the underlying crash, although such was not my intent and I'm not sure how I could have done so!), is that change has moved these crashes into some other function.  However -- I did change some mechanics a little, so it's not entirely inconceivable I fixed this somehow.

I'm not sure precisely where crashes would migrate after that change -- into drainCharBufferIntoAtom or atomizeSourceChars (note name change because a change after the rev noted above changed that name), maybe.  But merely typing either name into the box top right on crash-stats gives no results, which means either I'm misusing the API or there are no such crashes.

SO given no crashes in 63, and the two IMO-likeliest places for crashes to migrate have shown no migration, can we call this WORKSFORME now?  Will leave it to triage peoples to say if that's really the thing to do here.  It is dissatisfying to not know *what* happened here, but ¯\_(ツ)_/¯.

Comment 5

[Daniel Veditz [:dveditz]]

Still crashing in 63 release and 64 beta

Comment 6

[Liz Henry (:lizzard)]

Waldo, can you take another look at the new crashes?

Comment 7

[Andrew Overholt [:overholt]]

Ted took another look here and there's not much we have to go on.

Comment 8

[Ted Campbell [:tcampbell]]

There are some interesting GeckoView segfaults showing up on 67 such as: https://crash-stats.mozilla.org/report/index/72d88d1b-d1d2-47fe-a7e9-d0dc40190527
There is a bounds check but the type is char16_t which may be signed so seems invalid.

Jeff, as the person familiar with parser and C++ data types can you sanity check that this isn't completely wrong?

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Ted Campbell [:tcampbell] from comment #8)

There are some interesting GeckoView segfaults showing up on 67 such as: https://crash-stats.mozilla.org/report/index/72d88d1b-d1d2-47fe-a7e9-d0dc40190527
There is a bounds check but the type is char16_t which may be signed so seems invalid.

"char16_t - type for UTF-16 character representation, required to be large enough to represent any UTF-16 code unit (16 bits). It has the same size, signedness, and alignment as std::uint_least16_t, but is a distinct type. " -- https://en.cppreference.com/w/cpp/language/types

That says it should be unsigned, unless there's a compiler bug.

This smells more like some form of UAF or memory trashing, given the other crashes around here. Note that there are 5-6 crashes this week all from the same install time -- likely the same device, but (as happens) it reported the same crash multiple times.

Comment 10

[Jeff Walden [:Waldo]]

char16_t is definitely required to be unsigned, and we statically assert this.

Whatever the problem is here, that stack doesn't really help us at figuring it out. The true issue could be memory corruption; or it could be aggressive compiler inlining means the bounds-check gets eliminated unexpectedly; or it could be a compiler bug around optimization/inlining; or something else. But there's really nothing to go on so far. :-(

Comment 11

[David Lawrence [:dkl]]

Removing employee no longer with company from CC list of private bugs.

Comment 12

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.