715105 - AMF changes inject bug because Domain Env not set properly

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Library, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

ATS10AS3 broke after patches for Bug 673355 was integrated to player mainline.

(We didn't catch this because the tests that failed are not part of the set that are run automatically.)

Kapil has a fix in his player sandbox.

This bug is to stash conversation related to this bug and the patches for it, rather than tacking it onto Bug 673355 which already has over 75 comments.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: chat log with notes on reproducing test failure

Comment 2

[Kapil Raja Durga]

Attachment: Fix for AMF changes inject bug because Domain Env not set properly

Fix for injection bug 715105 which was caused due to incorrect domain Enviroment in registerClassAlias. A new virtual function getDomainEnv is introduced in the TopLevel class.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 585730 [details] [diff] [review]
Fix for AMF changes inject bug because Domain Env not set properly

Review of attachment 585730 [details] [diff] [review]:
-----------------------------------------------------------------

I've sent a review via email, but I'll copy the most relevant portion here.

::: core/Toplevel.h
@@ +57,5 @@
>      public:
>          static Toplevel* create(MMgc::GC* gc, AbcEnv* abcEnv);
>  
>          GCRef<builtinClassManifest> builtinClasses() const;
> +        virtual DomainEnv* getDomainEnv();

You should differentiate between Toplevel's interface with its callers and Toplevel's interface with its extending classes.  In particular: If you are introducing a virtual method like getDomainEnv() that is solely meant to be overridden by subclasses and *not* meant to be called directly from external clients, then that method should be declared virtual *and* private (or at least protected, which may or may not be necessary in this case).  C++, unlike Java, allows one to declare methods that are virtual and private, and I think that is called for in the coding pattern you are following.

 See also:

   http://www.gotw.ca/publications/mill18.htm

   http://www.parashift.com/c++-faq-lite/strange-inheritance.html#faq-23.4

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Oh, one more relevant snippet from the email:

Add documentation for the method you are adding.  State explicitly in its declaration in the Toplevel class definition that it is a hook that is meant to be overridden.  If you know what its contract is (i.e. what it assumes from its callers, and what it guarantees if those assumptions hold), then include that as part of the doc.  But if you are not sure, then don't make a contract up from thin-air; better to just tell the reader that its a hook and that the audience needs to do their own analysis to determine how it all holds together.

Comment 5

[Kapil Raja Durga]

Attachment: Fix for AMF changes inject bug because Domain Env not set properly

Changed function name from getDomainEnv to getDomainEnvOverridableHook. Changed this function from public virtual to private virtual.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 586007 [details] [diff] [review]
Fix for AMF changes inject bug because Domain Env not set properly

R+, with one caveat:

Regarding the reference to a bug number in the comment above the getDomainEnvOverridableHook() declaration, it would be better identify whether you are referencing Bugzilla or Watson, (especially since you are noting that this is to fix a player issue and so some people may start off looking at Watson).

I can make this change myself to the patch, since I assume I will be pushing this to tamarin-redux.

Comment 7

[Tamarin Bot]

changeset: 7150:cc625eba1f4c
user:      Kapil Raja Durga <[e-mail]>
summary:   Bug 715105: Fix for AMF changes inject bug because Domain Env not set properly (r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/cc625eba1f4c

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(reopening to ensure that everything lands in Brannan branch as necessary.)

Comment 9

[Tamarin Bot]

changeset: 7152:f1f6d0a8e749
user:      Felix S Klock II <[e-mail]>
summary:   Bug 715105: clarify bugzilla reference in comment (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/f1f6d0a8e749

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.