Assertion failure: types::TypeScript::ThisTypes(script)->hasType(types::Type::UndefinedType()) in TryEnablingIon

RESOLVED FIXED in Firefox 24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla25
x86_64
All
Points:
---

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed)

Details

(Whiteboard: [qa-], URL)

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 860838
(Assignee)

Comment 1

5 years ago
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...
Flags: needinfo?(dtc-moz)
(Assignee)

Updated

5 years ago
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.
Flags: needinfo?(dtc-moz)
(Assignee)

Comment 3

5 years ago
(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?
(Assignee)

Comment 4

5 years ago
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 → ---
(Assignee)

Updated

5 years ago
status-firefox24: --- → affected
status-firefox25: --- → affected

Updated

5 years ago
Attachment #766722 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/d0508b1725d6
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 7

5 years ago
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?

Updated

5 years ago
status-firefox25: affected → fixed

Updated

5 years ago
Attachment #766722 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.