Bug 156808 - WebScriptObject description swizzler should work in a multi-threaded world
Summary: WebScriptObject description swizzler should work in a multi-threaded world
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-20 14:05 PDT by Keith Miller
Modified: 2016-04-21 13:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.42 KB, patch)
2016-04-20 14:35 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (15.28 KB, patch)
2016-04-20 14:57 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (15.51 KB, patch)
2016-04-20 16:25 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-04-20 14:05:57 PDT
WebScriptObject description swizzler should work in a multi-threaded world
Comment 1 Keith Miller 2016-04-20 14:35:29 PDT
Created attachment 276853 [details]
Patch
Comment 2 Geoffrey Garen 2016-04-20 14:44:52 PDT
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.
Comment 3 Keith Miller 2016-04-20 14:57:02 PDT
Created attachment 276855 [details]
Patch for landing
Comment 4 mitz 2016-04-20 15:06:15 PDT
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.
Comment 5 Keith Miller 2016-04-20 16:25:40 PDT
Created attachment 276864 [details]
Patch
Comment 6 WebKit Commit Bot 2016-04-20 16:27:35 PDT
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.
Comment 7 Keith Miller 2016-04-20 16:28:46 PDT
> 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...
Comment 8 Geoffrey Garen 2016-04-20 16:30:17 PDT
Comment on attachment 276864 [details]
Patch

Man this patch just gets better and better :P.
Comment 9 Geoffrey Garen 2016-04-20 16:43:00 PDT
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 ]
Comment 10 Keith Miller 2016-04-20 17:11:30 PDT
(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.
Comment 11 WebKit Commit Bot 2016-04-21 13:59:42 PDT
Comment on attachment 276864 [details]
Patch

Clearing flags on attachment: 276864

Committed r199834: <http://trac.webkit.org/changeset/199834>
Comment 12 WebKit Commit Bot 2016-04-21 13:59:47 PDT
All reviewed patches have been landed.  Closing bug.