Bug 75296 – JSString should not have JS_EXPORTCLASS annotation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Hajime Morrita]

JSString is annotated JS_EXPORTCLASS but we should remove it because it's no longer a part of public (exported) API.
With this annotation, turning export macros on will result extra symbol exposure.

[Hajime Morrita]

Created attachment 120640 [details]
Patch

[Alexey Proskuryakov]

JSString is used in WebCore (see e.g. DOMWrapperWorld.h). Why does it matter that it's not part of a public API?

[Kevin Ollivier]

(In reply to comment #2)
> JSString is used in WebCore (see e.g. DOMWrapperWorld.h). Why does it matter that it's not part of a public API?

It is WebKit policy that APIs that are not public should export only the methods needed by WebCore, etc., to discourage third party developers from coding against those APIs.

Hajime's patch is not removing the JSString exports entirely, just decreasing what is exported to only what is needed.

[Kevin Ollivier]

Comment on attachment 120640 [details]
Patch

It looks like Windows needs the class declaration exported in order to provide access to JSString's vtable. I'd have to double-check, but I think the reason this patch works for other platforms is that gcc lets you add the class's vtable to the symbol exports file, but Windows does not. This is probably why we have JS_EXPORTCLASS and JS_EXPORTDATA macros even though we're using symbol export files.

[Kevin Ollivier]

(In reply to comment #4)
> (From update of attachment 120640 [details])
> It looks like Windows needs the class declaration exported in order to provide access to JSString's vtable. I'd have to double-check, but I think the reason this patch works for other platforms is that gcc lets you add the class's vtable to the symbol exports file, but Windows does not. This is probably why we have JS_EXPORTCLASS and JS_EXPORTDATA macros even though we're using symbol export files.

Sorry, by "Windows" I meant the Apple Windows port's compiler, that is, Visual C++.

[Hajime Morrita]

Oliver, thanks for your explanation and investigation.

> It looks like Windows needs the class declaration exported in order to provide access to JSString's vtable. I'd have to double-check, but I think the reason this patch works for other platforms is that gcc lets you add the class's vtable to the symbol exports file, but Windows does not. This is probably why we have JS_EXPORTCLASS and JS_EXPORTDATA macros even though we're using symbol export files
Interesting. 
And I didn't know that Apple Windows port already relies on the macro. 
I'll try another patch to examine my understanding. If if doesn't work, I'll close this bug as invalid.

[Hajime Morrita]

Created attachment 121408 [details]
An attempt to fix windows build break

[WebKit Review Bot]

Comment on attachment 121408 [details]
An attempt to fix windows build break

Clearing flags on attachment: 121408

Committed r104359: <http://trac.webkit.org/changeset/104359>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

FYI, this caused bug 76101

[Hajime Morrita]

(In reply to comment #10)
> FYI, this caused bug 76101
Oops. Thanks for fixing that.