Closed Bug 1272887 Opened 5 years ago Closed 5 years ago

win32 crash during GC when using PersistentRooted<GCVector>


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox49 --- affected
firefox50 --- fixed


(Reporter: till, Assigned: sfink)




(3 files, 1 obsolete file)

Attached patch test caseSplinter Review
Seems like PersistentRooted has a problem with GCVector, but only on win32. Attached is a test case that's as minimal as I can make it. It's just moving a few lines out of #IFDEF blocks for Promise. Which this is blocking :(

This reproduces easily in the shell by running `dist/bin/js -e "gc()"` for me, in a win10 VM.

Ni? sfink because we talked about this a few weeks ago.
Flags: needinfo?(sphink)
For reference, the conversation we had about this last month starts here and continues until ~the end of that day:
MozReview-Commit-ID: BMaft7pCX2q
Attachment #8759301 - Flags: review?(terrence)
Assignee: nobody → sphink
Sorry for taking so long to look into this. It's a totally easy to reproduce crash, I was just fighting with my Windows setup until I finally gave up and used Waldo's.
Flags: needinfo?(sphink)
Ok, finally resolved the MSVC breakage on the gecko tree. I still think it may be a bug that MSVC has a problem with aligning the ptr field, but so be it. DispatchWrapper is unlikely to be used anywhere else, and even if it is, an 8-byte alignment is probably a good idea anyway.
Attachment #8760476 - Flags: review?(terrence)
Attachment #8759301 - Attachment is obsolete: true
Attachment #8759301 - Flags: review?(terrence)
Attachment #8760476 - Flags: review?(terrence) → review+
Pushed by
Set alignment of PersistentRooted.ptr field for reinterpret_cast on win32, r=terrence
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
I didn't find anything in mfbt/Alignment.h that would work for my purposes, so how about a temporary local JS_ALIGNAS macro? It should more properly go into somewhere like jsutil.h, but I *hope* that this is a very temporary state and I didn't want to encourage more users.

One thing I'm unsure about is that it still uses alignas for clang, and says either that you need clang 3.3 to use alignas, or perhaps that you need clang 3.6 in order to know that you can use alignas. And in either case, I don't know what our minimum clang version is, given that the patch is working on osx on inbound already. Is this another case of running a newer compiler on CI?
Attachment #8761262 - Flags: review?(mh+mozilla)
I installed a local copy of clang 3.4 and tried straight alignas. It seems to compile, and it affected sizeof() as expected.
Attachment #8761262 - Flags: review?(mh+mozilla) → review?(jwalden+bmo)
Reopening until I resolve the compiler compatibility issue. Also switching review to Waldo since I think I've resolved the questions around alignas support, and now the review is more about how to do the temporary hack.
Resolution: FIXED → ---
Comment on attachment 8761262 [details] [diff] [review]
followup fix - alignas is not yet allowed

Till said he'd take this review.
Attachment #8761262 - Flags: review?(jwalden+bmo) → review?(till)
Comment on attachment 8761262 [details] [diff] [review]
followup fix - alignas is not yet allowed

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

r=me, that is indeed straight-forward enough for me to review.
Attachment #8761262 - Flags: review?(till) → review+
Pushed by
followup fix - alignas is not yet allowed, r=till
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1280789
Depends on: 1288925
You need to log in before you can comment on or make changes to this bug.