Restore back the log function in VMPI_spyCallback

RESOLVED FIXED in Q4 11 - Anza

Status

Tamarin
Garbage Collection (mmGC)
P4
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Ruchi Lohani, Assigned: Ruchi Lohani)

Tracking

unspecified
Q4 11 - Anza
x86
Windows 7
Bug Flags:
flashplayer-bug +

Details

(Whiteboard: has-patch, must-fix-candidate)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
The VMPI_spyCallback in SpyUtilPosix.cpp does this:
RedirectLogOutput(SpyLog);
MMgc::GCHeap::GetGCHeap()->DumpMemoryInfo();
RedirectLogOutput(NULL);

Given that we have a RedirectLogOutput() function which could have been used to set a redirect log function previously, we should be restoring back the previous log function rather than setting it to NULL. The same issue is also present in other Spy utils.

Updated

8 years ago
Assignee: nobody → rulohani
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → flash10.x - Serrano
(Assignee)

Comment 1

8 years ago
Created attachment 488931 [details] [diff] [review]
Restore back the old log function
Attachment #488931 - Flags: superreview?
Attachment #488931 - Flags: review?(treilly)
(Assignee)

Updated

8 years ago
Attachment #488931 - Flags: superreview? → superreview?(lhansen)
(Assignee)

Comment 2

8 years ago
I also had a thought that we can probably avoid having a new Get function for the loggging. RedirectLogOutput() can be modified to return the old log function which can then be used to reset it at the end. Though it might require the RedirectLogOutput to be renamed to something else.

Comment 3

8 years ago
Comment on attachment 488931 [details] [diff] [review]
Restore back the old log function

Tabstops in the file should be cleaned up.
Attachment #488931 - Flags: superreview?(lhansen) → superreview+
(Assignee)

Comment 4

8 years ago
Created attachment 489338 [details] [diff] [review]
Updated patch.
Attachment #488931 - Attachment is obsolete: true
Attachment #489338 - Flags: review?(treilly)
Attachment #488931 - Flags: review?(treilly)
(Assignee)

Updated

8 years ago
Attachment #489338 - Flags: review?(treilly)
(Assignee)

Updated

8 years ago
Attachment #489338 - Flags: review?(treilly)

Comment 5

8 years ago
Comment on attachment 489338 [details] [diff] [review]
Updated patch.

Don't like the code duplication, please move the typedef, getter and logFunc to a common file.   In general code in a platform specific file should be platform specific although and not a copy from other platform specific files (I realize other certain large code bases don't follow this rule but it would be nice to adhere to it in the vm).
Attachment #489338 - Flags: review?(treilly) → review-
(Assignee)

Comment 6

8 years ago
RedirectLogOutput() also falls in the same duplication category then - common code in all platform specific files. Is there an existing common file where these could be moved?
(In reply to comment #6)
> RedirectLogOutput() also falls in the same duplication category then - common
> code in all platform specific files. Is there an existing common file where
> these could be moved?

I imagine if a file for it exists, it should be in vmbase/.  But I do not see a good .cpp in there for this (that directory is a relatively new addition to the infrastructure).
(Assignee)

Comment 8

8 years ago
Created attachment 501524 [details] [diff] [review]
Patch v2

Take 2, with logging functions common to all platforms moved to a separate file.
Attachment #489338 - Attachment is obsolete: true
Attachment #501524 - Flags: review?(treilly)

Comment 9

8 years ago
Comment on attachment 501524 [details] [diff] [review]
Patch v2

Looks good, my only concern is that GetCurrentLogFunction, LoggingFunction and RedirectLogOutput should be scoped somehow, ie put them in the vmbase namespace.

Updated

8 years ago
Attachment #501524 - Flags: review?(treilly) → review-
(Assignee)

Comment 10

8 years ago
Created attachment 504805 [details] [diff] [review]
Patch v3

Updated the scope of the log functions. I just used "using namespace vmbase;", let me know if I should instead just use the vmbase::RedirectlogOutput() etc. Also, the xcode and visual studio project files will need to be changed to add the 2 new files. Should those also be attached to the bug?
Attachment #501524 - Attachment is obsolete: true
Attachment #504805 - Flags: review?(treilly)

Updated

8 years ago
Attachment #504805 - Flags: review?(treilly) → review+
(Assignee)

Updated

8 years ago
Attachment #504805 - Flags: superreview?(lhansen)

Comment 11

8 years ago
vmbase is not a dumping ground for code that's common to platform specific files.  So far it has been a place to put code that must be shared between MMgc and AVMPlus because those are compiled separately by the Flash Player.  As Tommy observes, we have no place (yet) to put code that's common in the various platform files.  I'm not particularly allergic to code that's duplicated in the various VMPI implementation files, especially trivialities like these.

SR+, but I'd like to solicit opinions from Steven and Edwin before it lands.

Updated

8 years ago
Attachment #504805 - Flags: superreview?(lhansen) → superreview+

Updated

8 years ago
Attachment #504805 - Flags: superreview+ → superreview?(edwsmith)

Comment 12

8 years ago
I generally agree with Lars, but IMHO the patch needs cleanup:

One must-fix issue: declaring "extern LoggingFunction logFunc" in a cpp file is a recipe for pain down the road; extern declarations really *must* live in a header file.

Style nits on this patch:

-- We have a GetCurrentLogFunction(), so why do we also need to expose the global variable?
-- if logFunc is going to be global, use the naming convention used elsewhere, "gLogFunc" or "g_logFunc".
OS: Mac OS X → Windows 7

Comment 13

8 years ago
Comment on attachment 504805 [details] [diff] [review]
Patch v3

Agree with Lars + Steven.  if this is trivial code then lets just duplicate it and not pollute vmbase.  if its nontrivial then lets find a good place to share it between platforms.  (/platform?)
Attachment #504805 - Flags: superreview?(edwsmith) → superreview-

Updated

8 years ago
Flags: flashplayer-bug+
Whiteboard: has-patch

Updated

8 years ago
Whiteboard: has-patch → has-patch, must-fix-candidate

Updated

8 years ago
Priority: P3 → P4

Updated

7 years ago
Target Milestone: Q3 11 - Serrano → Q4 11 - Anza

Updated

7 years ago
Assignee: rulohani → nobody

Updated

7 years ago
Assignee: nobody → rulohani
(Assignee)

Comment 14

7 years ago
Created attachment 557342 [details] [diff] [review]
Patch v4

Moved the Logging methods to the GenericPortUtils.cpp in VMPI.
Attachment #504805 - Attachment is obsolete: true
Attachment #557342 - Flags: review?(treilly)

Updated

7 years ago
Attachment #557342 - Flags: review?(treilly) → review+
(Assignee)

Updated

7 years ago
Attachment #557342 - Flags: superreview?(lhansen)

Updated

7 years ago
Attachment #557342 - Flags: superreview?(lhansen) → superreview+
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 15

7 years ago
changeset: 6559:8f002fb91ddd
user:      Ruchi Lohani<rulohani@adobe.com>
summary:   Bug 592187 - Restore back the log function in VMPI_spyCallback (r=treilly, sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/8f002fb91ddd
You need to log in before you can comment on or make changes to this bug.