Bug 28054 – use derefInNotNull function to work around winscw compiler bug regarding template arguments.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Yongjun Zhang]

In the following code:

-----------------------------------------------------
class MyTemplatedArgument;

class ContainerClass {

private:
   PassRefPtr<MyTemplatedArgument> t;
};
-----------------------------------------------------

Winscw compiler will complain that MyTemplatedArugment is not defined so that it couldn't find the deref() implementation of MyTemplatedArgument. Forward declaration doesn't help. In previous S60 port, we had to explicitly include the MyTemplatedArugment.h before ContainerClass declaration to resolve this issue.  This leads to a lot of changes in WebCore files and some requires moving around header files.

This is because PassRefPtr's dtor is inlined and the MyTemplatedArguemnt::deref() is called inside PassRefPtr's dtor, and the compiler tries to further resolve deref() which is not visible yet if the header file is not included.

To avoid changing large number of WebCore files just to get around this compiler issue, we can use an helper function to defer resolving deref().  We flag this changes in COMPILER(WINSCW) to avoid any possible performance hit in hardware.

[Yongjun Zhang]

Created attachment 34218 [details]
Fix the issue with a helper function.

Use a helper function to work around winscw compiler forward declaration bug regarding templated classes.

Add parenthesis around (PassRefPtr::*UnspecifiedBoolType) to make winscw compiler
work with the default UnSpecifiedBoolType() operator, which removes the winscw
specific bool cast hack.

[Darin Adler]

Comment on attachment 34218 [details]
Fix the issue with a helper function.

> +#if COMPILER(WINSCW)
> +    template <typename T> void releaseRef(T*);
> +#endif

This doesn't need to be #ifdef'd compiler-specific. We can use this with the other compilers too. But it needs to be an inline function. It will hurt performance to not have it inlined. A good name for it would be derefIfNonNull or safeDeref.

Since this is designed to be used only internally, we should give it a name that won't tempt anyone to use it. And probably put it in a different namespace, as is done with WTF::Internal in FastAllocBase.h.

releaseRef is not a good name for this -- it's already the name for an unrelated member function.

> +#if COMPILER(WINSCW)
> +    template <typename T> void releaseRef(T* ptr)
> +    {
> +        if (UNLIKELY(ptr != 0))
> +            ptr->deref();
> +    }
> +#endif

This should be marked inline and probably also should be defined before the PassRefPtr template just in case order matters. For non-template code it can sometimes be important to define inline functions before using them.

Otherwise looks quite good.

review- because of the comments above.

[Yongjun Zhang]

Created attachment 35070 [details]
Modified patch as per Darin's comments.

Thanks for the review!

I modified the patch according to the comments.  For winscw compiler, if inline is present in derefInNotNull templated function definition, the compiler will try to resolve deref() and will eventually report error if the argument class T is not defined before PassRefPtr<T> is used.  Therefore, I had to remove inline for winscw compiler, and add inline for other compilers to avoid introducing any performance hit.

[Eric Seidel (no email)]

Comment on attachment 35070 [details]
Modified patch as per Darin's comments.

I think we need comments in the code to document what these two work-arounds do.

One about the forward declaration. And one about the need for the ().

[Yongjun Zhang]

Created attachment 35143 [details]
add comments in the code to explain why these two changes are needed.

[Eric Seidel (no email)]

Comment on attachment 35143 [details]
add comments in the code to explain why these two changes are needed.

Looks great!

typo:
 174     // which will fail compiling when PassRefPtr<T> is used as class member or fucntion 

if you were a committer I would r+ this and you could fix the typo when landing.

[Darin Adler]

+#if COMPILER(WINSCW)
+    // Remove inline here to prevent winscw compiler agressively resolving T::deref(),
+    // which will fail compiling when PassRefPtr<T> is used as class member or fucntion 
+    // arguments before T is defined.
+    template<typename T> void derefIfNotNull(T* ptr)
+#else
+    template<typename T> inline void derefIfNotNull(T* ptr)
+#endif    

I think you should put the ifdef around only the word "inline".

I think the definition of the derefIfNotNull function template should come before the definition of the PassRefPtr class, since inlining is much more likely if they are in that order. You don't even need a separate declaration and definition.

[Yongjun Zhang]

Created attachment 39056 [details]
Use derefIfNotNull for RefPtr, as it was done in PassRefPtr.

[Janne Koskinen]

This needs bit more tweaking.

Every call of ptr->ref() and ptr->deref() from inlined function in refptr.h and passrefptr.h must call non-inlined function instead (derefIfNotNull/refIfNotNull). This way we can get away of using extra includes in all webkit code with exception of threading functions.

[Eric Seidel (no email)]

Comment on attachment 39056 [details]
Use derefIfNotNull for RefPtr, as it was done in PassRefPtr.

LGTM.

[Eric Seidel (no email)]

Comment on attachment 39056 [details]
Use derefIfNotNull for RefPtr, as it was done in PassRefPtr.

Rejecting patch 39056 from commit-queue.

This patch will require manual commit. WebKitTools/Scripts/build-webkit failed with exit code 1

[Eric Seidel (no email)]

Sadly build logs are not yet available from the commit-bot.  But it appears this patch does not build on Mac in Release mode.

[Yongjun Zhang]

(In reply to comment #12)
> Sadly build logs are not yet available from the commit-bot.  But it appears
> this patch does not build on Mac in Release mode.

Thanks a lot!

I will check why it fails in Mac build in Release mode.

[Yongjun Zhang]

Created attachment 39298 [details]
fix build break in mac build.

Thanks a lot for review.

The reason it fails the mac build is JSObjectWrapper::ref() and deref() are defined as private methods in WebCore/bridge/jni/jni_instance.h.  The new patch moves them to public to fix the build break.

Is there any reason why ref() and deref() needs to be private in this case?  If yes, the patch might be wrong and I will figure out a different way to address this.

[Janne Koskinen]

Created attachment 39431 [details]
what is still required

For refptr.h/passrefptr.h to work every inline function containing either ref() or deref() needs to be changed to non-inlined.
This is now addition to already commited patch that added the derefIfNotNull() - function.

[Darin Adler]

> For refptr.h/passrefptr.h to work every inline function containing either ref()
> or deref() needs to be changed to non-inlined.

Won't that hurt performance?

[Yongjun Zhang]

(In reply to comment #16)
> > For refptr.h/passrefptr.h to work every inline function containing either ref()
> > or deref() needs to be changed to non-inlined.
> 
> Won't that hurt performance?

Thanks for the comments.

Yes, it will hurt performance.  Good thing is winscw compiler is only used for building apps running on S60 emulator, and for real hardware, RVCT (armcc) compiler is used, so we wouldn't see performance penalty in real hardware because the inlined version is used for RVCT.

That being said, I don't think it is a good idea to change every inlined ref&deref to be non-inlined. Two reasons:

1. it makes the code a bit harder to read; 
      if (ptr)
          ptr->deref();
      is much more readable than:
         derfIfNotNull(ptr);

2. even in emulator, we still want performance hit to be as small as possible; therefore, the patch only changes ref&deref to be non-inline at places where it is really necessary.

[Janne Koskinen]

> 1. it makes the code a bit harder to read; 

I think this is matter of opinion. I made it to be consistent all for all functions.
If wanted it can be changed e.g.
if (ptr) derefInternal(ptr); // or similar

> 2. even in emulator, we still want performance hit to be as small as possible;
> therefore, the patch only changes ref&deref to be non-inline at places where it
> is really necessary.

I don't care about the emulator performance at all as it hopefully will not <fingers crossed> live too long.

Yes, it is not needed for every function, but I know that some of the constructors, copy constructors and assignment operators will need the modification as well.
You have now covered destructors and clear() (in the new patch).

[Eric Seidel (no email)]

Comment on attachment 39298 [details]
fix build break in mac build.

This patch looks OK.  It would be better with references to the winscw bugs.  You mention above that winscw might not be used for much longer?  We should have bugs on file about rolling out these kind of hacks once winscw is no longer used.

Have you filed a bug with the winscw team about this issue?  Will you please add a link to said bug to the ChangeLog so someone looking at this later might know if they can remove this hack?

[Yongjun Zhang]

Created attachment 40132 [details]
modify the patch to include Eric's review comments.

thanks for the comments, Eric.

I added the winscw compiler bug url in the ChangeLog and created another webkit bug (https://bugs.webkit.org/show_bug.cgi?id=29738) to remind us to revert the changes when winscw compiler bug is fixed.

[Eric Seidel (no email)]

Comment on attachment 40132 [details]
modify the patch to include Eric's review comments.

OK.  I'm OK with this hack for now.  Seeing cq+ since Yongjun is not a committer to my knowledge.

[Eric Seidel (no email)]

Thank you for following up with the bugs btw.  I think it's important that we document this sort of thing for long-term code cleanliness. :)

[Yongjun Zhang]

(In reply to comment #22)
> Thank you for following up with the bugs btw.  I think it's important that we
> document this sort of thing for long-term code cleanliness. :)

Thanks a lot, Eric.

I absolutely agree;  it is a good practice to document those temporary hacks to avoid clattering WebKit source code in long run.  I also find it is very helpful to tracking down those changes in the future.  Maybe we could add this practice to  WebKit coding guidelines?

[WebKit Commit Bot]

Comment on attachment 40132 [details]
modify the patch to include Eric's review comments.

Rejecting patch 40132 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
criptCore/wtf/RefPtr.h
	M	WebCore/ChangeLog
	M	WebCore/bridge/jni/jni_instance.h
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/WebCore/bridge/jni/jni_instance.h

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

[Yongjun Zhang]

Created attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

 
Maybe it would be better to modify WebKitTools/Script/svn-create-patch in a way that it can remove Tabs in the resulting patch.

[Eric Seidel (no email)]

Comment on attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

svn-create-patch should certainly at least warn about tabs.  (bug 29509)  Having a --remove-tabs option wouldn't necessarily be bad either.

[WebKit Commit Bot]

Comment on attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

Rejecting patch 40239 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11337 test cases.
http/tests/xmlhttprequest/cross-origin-no-authorization.html -> failed

Exiting early after 1 failures. 8972 tests run.
259.20s total testing time

8971 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

[Eric Seidel (no email)]

Comment on attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

Flakey test. bug 29322.

[WebKit Commit Bot]

Comment on attachment 40239 [details]
remove the Tab in previous patch, please review, thanks.

Clearing flags on attachment: 40239

Committed r48825: <http://trac.webkit.org/changeset/48825>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

This change is broken for two reasons (one serious, one less so):
1) RefPtr.h uses a function named derefIfNotNull which is not defined anywhere.  It frequently works when it picks up a definition from PassRefPtr.h, but there's no requirement that that header be included.
2) Adding function call overhead to every deref is a terrible idea.

Reason 1 will cause build failures so this change needs to be rolled out.

[Mark Rowe (bdash)]

Rolled out in r48839.  Please revise the patch to address at least the first issue and resubmit for review.

[Eric Seidel (no email)]

Thank you Mark.

[Eric Seidel (no email)]

Comment on attachment 40132 [details]
modify the patch to include Eric's review comments.

This patch was rolled out, marking r-.

[Yongjun Zhang]

Created attachment 40736 [details]
include PassRefPtr.h explicitly to avoid possible building break.

Mark and Eric, thank you both for the comments.

I copied the idea from adoptRef<T>() in PassRefPtr.h when introducing derefIfNotNull(); and I assume if adoptRef doesn't break the build, neither will derefIfNotNull().

To be safe, I modified the patch and explicitly include PassRefPtr.h to prevent possible building break.

Regarding the concern on performance hit, the patch only removes inline for winscw compiler; for other compilers, derefIfNotNull is still inlined.

[Adam Barth]

Comment on attachment 40736 [details]
include PassRefPtr.h explicitly to avoid possible building break.

Why is there a random change to jni_instance in this patch?

[Janne Koskinen]

(In reply to comment #36)
> (From update of attachment 40736 [details])
> Why is there a random change to jni_instance in this patch?

Well, it's not random but maybe it should be a separate patch.

[Janne Koskinen]

Could this be fixed ? webkit won't compile for s60 emulator environment before this issue is resolved.

[Tor Arne Vestbø]

Created attachment 48476 [details]
Rebased against trunk, built successfully for both Safari/Mac and Qt/Mac

[WebKit Review Bot]

Attachment 48476 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/RefPtr.h:60:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tor Arne Vestbø]

(In reply to comment #40)
> Attachment 48476 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> JavaScriptCore/wtf/RefPtr.h:60:  More than one command on the same line 
> [whitespace/newline] [4]
> Total errors found: 1

Seems we have quite a few of these already in header files:

[whopper:~/dev/webkit/wip] $ git grep -E ') {.*; [A-Za-z]+' -- **/*.h | wc -l
270

Eric, would it make sense to skip this check for headers, like we do for macros and switch cases?

[Tor Arne Vestbø]

Created attachment 48480 [details]
Split up removal of bool hack into separate patch

[Tor Arne Vestbø]

Created attachment 48481 [details]
Split up drefIfNotNull() patch

[WebKit Review Bot]

Attachment 48481 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/RefPtr.h:60:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 48481 [details]
Split up drefIfNotNull() patch

Treating the style as false positive and considering that this is the same as r47592, it looks good to me.

[Eric Seidel (no email)]

Did this land?  This has been in the pending-commit list for months. :(

[WebKit Commit Bot]

Comment on attachment 48480 [details]
Split up removal of bool hack into separate patch

Rejecting patch 48480 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--force']" exit_code: 1
Parsed 2 diffs from patch file(s).
patching file JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file JavaScriptCore/wtf/RefPtr.h
Hunk #1 FAILED at 65.
1 out of 1 hunk FAILED -- saving rejects to file JavaScriptCore/wtf/RefPtr.h.rej

Full output: http://queues.webkit.org/results/3972045

[WebKit Commit Bot]

Comment on attachment 48481 [details]
Split up drefIfNotNull() patch

Rejecting patch 48481 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--force']" exit_code: 1
Last 500 characters of output:
/wtf/RefPtr.h.rej
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/bridge/jni/jsc/JavaInstanceJSC.h
Hunk #1 FAILED at 50.
Hunk #2 succeeded at 57 with fuzz 2.
1 out of 2 hunks FAILED -- saving rejects to file WebCore/bridge/jni/jsc/JavaInstanceJSC.h.rej
patching file WebCore/bridge/jni/v8/JavaInstanceV8.h
Hunk #1 FAILED at 50.
Hunk #2 succeeded at 57 with fuzz 2.
1 out of 2 hunks FAILED -- saving rejects to file WebCore/bridge/jni/v8/JavaInstanceV8.h.rej

Full output: http://queues.webkit.org/results/3967060

[Eric Seidel (no email)]

I'm suspecting these were already landed.  If they weren't please don't reopen this unless you plan do land it.  :)