Closed
Bug 530945
Opened 16 years ago
Closed 15 years ago
resolveSignatures happens too soons, preventing valid loading
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P2)
Tamarin Graveyard
Virtual Machine
Tracking
(Not tracked)
VERIFIED
FIXED
flash10.1
People
(Reporter: stejohns, Assigned: stejohns)
Details
Attachments
(1 file, 3 obsolete files)
3.29 KB,
application/zip
|
Details |
Jeff Dyer reports:
"The method ‘Traits::resolveSignatures’ is being called on all ABC blocks during ‘AvmCore::prepareActionPool’ (prepare), while it used to not be called until ‘MethodEnv::coerceEnter’ was invoked (enter). This change in behavior was introduced with traits caching in fall 08. In effect, what used to be a verify time error has become a load time error.
This change affects global bindings that have forward type references across ABC boundaries in code that gets /entered/ after the forward referenced definitions are /prepared/. (such code use to be acceptable to the VM but is no longer.) In the AIR app (ImageSizer) the type reference in question is in a DoABC tag which does not get /entered/ before the defining tag is /prepared/.
The bug is not reproduceable in the stand-alone AVM because global code is prepared and entered before subsequent ABCs are prepared and entered. So a forward reference of this kind has always resulted in an error. (Forward references within an ABC are allowed) This is not the case in the Player (and AIR) because ABC blocks are queued up (prepared) until a ShowFrame tag is encountered, at which time they are entered in order. So unless a ShowFrame tag occurs between the reference and the referent DoABC tags, no error would have occurred before this change."
Assignee | ||
Comment 1•16 years ago
|
||
testcase that (hopefully) reproduces this in avmshell. jeff reports:
* makeswf doesn’t set the lazy flag, so I turn it on full-time (good enough for producing the error, code never runs of course)
* DoABCtag in the PlayerAvmCore parses and prepares the ABC, so I’ve hacked SWF parsing in AVM to do the same. Probably should make this the de facto behavior in lazy mode
* t.swf contains two DoABC tags containing the byte code from t.as and u.as in that order
* there is very possibly more going on in FP than there is in my hacked AVM, so proceed with caution.
* compiling test case: asc u.as; asc –import u.abc t.as; avmshell makeswf.abc — t.abc u.abc
Assignee | ||
Comment 2•16 years ago
|
||
Here's a rough initial possible fix... hasn't yet been tested against the original testcase, but it passes acceptance tests and the artificial test Jeff cooked up.
It's not ideal, but may be acceptable as a first cut... basically, the problem is that TraitsBindings contain some information that we can gather pre-resolve (eg methodCount, slotCount, bindings) and some we can only gather post-resolve (eg slot type, slot offsets, method types). In hindsight, TraitsBindings should probably be split into two chunks, but that's not what this patch does: instead, it allows TraitsBindings to be constructed prior to resolveSignature, but only fills in the info that it can. If you try to access non-filled-in info, you'll get assertions.
While this works, it's awkward and philosophically unappealing. It also necessitated adding an extra field (declaringScope) back into Traits -- strictly speaking, this might not be *necessary*, but getting builtins bootstrapped without it is exquisitely painful.
What this lacks prior to landing:
-- obviously, testing in the actual flash/air testcases that are failing (I'll try to do this over the long weekend)
Additional things that may not prevent a short-term landing (as this is a fix of high urgency) but may require addressing afterwards:
-- conceptual review to see if there's a cleaner way to do this... is it worth splitting TraitsBindings in two?
-- we need testcases in avmshell that can exercise this (avmshell will need a bit of augmenting to allow this; jeff's testcase hacks the code directly but adding a command-line option is probably in order, or perhaps hacking makeswf.abc to set the proper flag)
Assignee: nobody → stejohns
Attachment #414403 -
Flags: superreview?(edwsmith)
Attachment #414403 -
Flags: review?(jodyer)
Assignee | ||
Comment 3•16 years ago
|
||
Revised patch -- basically the same with a few bugs fixed. This does in fact appear to fix the issue at hand. Please review.
Attachment #414403 -
Attachment is obsolete: true
Attachment #415049 -
Flags: superreview?(edwsmith)
Attachment #415049 -
Flags: review?(jodyer)
Attachment #414403 -
Flags: superreview?(edwsmith)
Attachment #414403 -
Flags: review?(jodyer)
Comment 4•16 years ago
|
||
if this fixes the urgent problem, then tentative R+ for pushing, but i'd like to take a bit longer to review to think about problems or simpler alternatives.
I think the JIST of what we want and what this fix implements, is that the Traits::linked flag (and related fields) and the existence and contents of TraitsBindings, are simply independent. TB is just a caching mechanism... nothing should change (besides cache churn) if we just created an instance of TB permanently inside Traits.
If some generic info (available when traits->linked == false) is in TB because it's rarely accessed, thats fine, we just have an if (linked) block in buildTraitsBindings for the stuff that shouldn't exist until after resolveSignatures is called the first time.
anything in TB that's very hotly accessed (is declaringScope hot?) probably should move into Traits anyway.
Assignee | ||
Comment 5•16 years ago
|
||
(In reply to comment #4)
> if this fixes the urgent problem, then tentative R+ for pushing, but i'd like
> to take a bit longer to review to think about problems or simpler alternatives.
agreed. it does fix an urgent problem, but isn't ideal.
> I think the JIST of what we want and what this fix implements, is that the
> Traits::linked flag (and related fields) and the existence and contents of
> TraitsBindings, are simply independent. TB is just a caching mechanism...
> nothing should change (besides cache churn) if we just created an instance of
> TB permanently inside Traits.
yep... the complication is that some fields of the current TB structure can't be populated until Traits::linked is true (ie, post-resolve).
Comment on attachment 415049 [details] [diff] [review]
Revised patch
I don't have the depth of understanding how traits are resolved to know if this solution is optimal, but I have confirmed that it fixes the test case
Attachment #415049 -
Flags: review?(jodyer) → review+
Comment 7•16 years ago
|
||
Comment on attachment 415049 [details] [diff] [review]
Revised patch
<squish> rubber stamp oozing ink.
we gotta refactor this so its painfully clear that the TraitsBinding caching code is independent from which fields are valid or not before/after linking happens, and finally that linking cant happen earlier than in previous releases.
Attachment #415049 -
Flags: superreview?(edwsmith) → superreview+
Assignee | ||
Comment 8•16 years ago
|
||
bumping to remind myself to push this change once redux is once again open for pushes
Assignee | ||
Comment 9•16 years ago
|
||
Comment on attachment 415049 [details] [diff] [review]
Revised patch
http://hg.mozilla.org/tamarin-redux/rev/2018762758ce
Attachment #415049 -
Attachment is obsolete: true
Assignee | ||
Comment 10•16 years ago
|
||
Previous patch to this bug broke -Dverifyall. This fixes it.
Attachment #418749 -
Flags: review?(jodyer)
Attachment #418749 -
Flags: review?(jodyer) → review+
Assignee | ||
Comment 11•16 years ago
|
||
Comment on attachment 418749 [details] [diff] [review]
Fix verifyall
http://hg.mozilla.org/tamarin-redux/rev/3ecfd171278d
Attachment #418749 -
Attachment is obsolete: true
Flags: flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 12•15 years ago
|
||
Is it possible to have an avmshell testcase that does not rely on hacking shell/swf.cpp? I'd like to add a regression testcase to the suite, but am not sure how to implement that.
You need to log in
before you can comment on or make changes to this bug.
Description
•