Bug 156808 – WebScriptObject description swizzler should work in a multi-threaded world

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

WebScriptObject description swizzler should work in a multi-threaded world

[Keith Miller]

Created attachment 276853 [details]
Patch

[Geoffrey Garen]

Comment on attachment 276853 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276853&action=review

r=me

> Source/WebCore/bridge/objc/objc_instance.mm:472
> +    if (s_descriptionDepth && s_descriptionDepth->isSet() && **s_descriptionDepth)

s_descriptionDepth should never be null here. Let's not test for null.

> Source/WebCore/bridge/objc/objc_instance.mm:488
> +    IMP currentDescription = method_setImplementation(method, (IMP)swizzleNSObjectDescription);
> +    if (currentDescription != (IMP)swizzleNSObjectDescription)
> +        s_originalNSObjectDescription = currentDescription;

It seems like it would be better to move the swizzling into the call_once. That way, we swizzle once, but if the user program does its own swizzling, we won't break it or cause ABA problems.

> Tools/TestWebKitAPI/Tests/mac/WebScriptObjectDescription.html:6
> +            if (base + "" === "<NSOBject>")

Should be NSObject, not NSOBject.

> Tools/TestWebKitAPI/Tests/mac/WebScriptObjectDescription.html:9
> +                return false;
> +        }
> +        return true;

These return values are backwards. The test only passes because the check is wrong too.

> Tools/TestWebKitAPI/Tests/mac/WebScriptObjectDescription.mm:46
> +            CRASH();

We don't want test failure to crash.

[Keith Miller]

Created attachment 276855 [details]
Patch for landing

Comment on attachment 276855 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=276855&action=review

> Source/WebCore/bridge/objc/objc_instance.mm:485
> +        s_originalNSObjectDescription = method_setImplementation(method, (IMP)swizzleNSObjectDescription);

This is race-prone. The swizzled implementation could get called, and try to load from s_originalNSObjectDescription, before the store to that variable is done. I think you should use method_exchangeImplementations instead.

[Keith Miller]

Created attachment 276864 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/bridge/objc/objc_instance.mm:475:  The parameter name "s_descriptionDepth" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


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

[Keith Miller]

> ERROR: Source/WebCore/bridge/objc/objc_instance.mm:475:  The parameter name
> "s_descriptionDepth" adds no information, so it should be removed. 
> [readability/parameter_name] [5]

I don't even know what happened here...

[Geoffrey Garen]

Comment on attachment 276864 [details]
Patch

Man this patch just gets better and better :P.

[Geoffrey Garen]

Not sure if this is your patch or not:

Unexpected flakiness: text-only failures (1)
  fast/replaced/table-percent-height.html [ Failure Pass ]


Regressions: Unexpected text-only failures (4)
  css2.1/t0905-c5525-flthw-00-c-g.html [ Failure ]
  css2.1/t0905-c5526-flthw-00-c-g.html [ Failure ]
  fast/canvas/set-colors.html [ Failure ]
  fast/text/emoji.html [ Failure ]

[Keith Miller]

(In reply to comment #9)
> Not sure if this is your patch or not:
> 
> Unexpected flakiness: text-only failures (1)
>   fast/replaced/table-percent-height.html [ Failure Pass ]
> 
> 
> Regressions: Unexpected text-only failures (4)
>   css2.1/t0905-c5525-flthw-00-c-g.html [ Failure ]
>   css2.1/t0905-c5526-flthw-00-c-g.html [ Failure ]
>   fast/canvas/set-colors.html [ Failure ]
>   fast/text/emoji.html [ Failure ]

Looks like it was just the ToT.

[WebKit Commit Bot]

Comment on attachment 276864 [details]
Patch

Clearing flags on attachment: 276864

Committed r199834: <http://trac.webkit.org/changeset/199834>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.