We have a report of assertion failures in TryEnablingIon. This shouldn't happen, since that information is persistent and shouldn't dissappear even with gc's. From Douglas Crosher [:dougc] > Noted an assertion failure in TryEnablingIon. The call to TryEnablingIon is > from InvokeFromAsmJS_Ignore. > > JS_ASSERT(types::TypeScript::ThisTypes(script)->hasType(types::Type:: > UndefinedType())); > > Should this not have happened, or should it just be giving up on enabling > the fast exit to Ion code in this case? > Hit this assertion running the Citadel demo on a x64 build. The function it > fails on is _alBufferData from > http://www.unrealengine.com/html5/UDKGame-Browser-Shipping.js lineno 5149. > Even if the 'this' type assertion is ignored, it fails on the following > argument type checks. > Is this type information persistent? Might it have been cleared by a GC? I have tried to create a testcase, but failed. So I cannot reproduce this assertation. I also build myself a debug version on x64 and couldn't reproduce either. Could you give more information or if your build failed every time, test again?
So I've tried different browsers, different browser runs and citadel and banana bread, but I wasn't able to trigger this issue. I also reasoned about it and checked. We definitely shouldn't hit this, since the argument types are only cleared when zone isn't active (i.e. no script on the stack). But here there are definitely scripts on the stack. So I think this assert was possible due to other bugs. I think it is safe to close this. If there is still an issue, I think the fuzzers will be able to find it...
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
Thank you for looking into it. This problem is reproducible here on a nightly x64 debug build, running the Citadel demo, and I'll keep an eye on it.
(In reply to Douglas Crosher [:dougc] from comment #2) > Thank you for looking into it. This problem is reproducible here on a > nightly x64 debug build, running the Citadel demo, and I'll keep an eye on > it. Oh that's annoying that I can't repro here. Can you give more details? Platform? Testing with clean profile?
Created attachment 766722 [details] [diff] [review] Patch Still not liking the fact that I haven't been able to repro this. As a result I haven't found the underlaying reason why the type information is gc'ed. But enabling ion when types don't correspond is very bad. Jit code makes assumptation about the argument types!
Assignee: general → hv1989
Status: RESOLVED → REOPENED
Attachment #766722 - Flags: review?(luke)
Resolution: INVALID → ---
status-firefox24: --- → affected
status-firefox25: --- → affected
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago → 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 766722 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 860838 User impact if declined: This is a safety improvement. We have no reports of crashes or wrong behaviour. But we have one report that we hit this debug assert. Going further in the code when that assumption failed could potentially be disastrous. Therefore going the safe route and make it impossible to go further without those conditions. Testing completed (on m-c, etc.): m-i: 2 days, m-c: 1 day Risk to taking this patch (and alternatives if risky): String or IDL/UUID changes made by this patch:
Attachment #766722 - Flags: approval-mozilla-aurora?
Attachment #766722 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox24: affected → fixed
Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.
You need to log in before you can comment on or make changes to this bug.