Bug 133559 – [iOS] Amazon app: Cannot interact with product page after tapping on product image

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Daniel Bates]

In the Amazon app for iOS, tapping on a product image makes the product page unresponsive to subsequent taps.

[Daniel Bates]

<rdar://problem/16332749>

[Daniel Bates]

Created attachment 232588 [details]
Patch

As a workaround, ignore "use strict" directive in JavaScript scripts loaded in the Amazon app for versions of the Amazon app linked against UIKit before iOS 8.

[Oliver Hunt]

Comment on attachment 232588 [details]
Patch

is navigator/geolocation readonly in other browsers?

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 232588 [details])
> is navigator/geolocation readonly in other browsers?

From observation in Google Chrome for Mac version 36.0.1985.49 beta, navigator.geolocation is readonly as I wasn't able to override it. Unlike Safari with a ToT WebKit, Chrome doesn't seem to throw a JavaScript type error when overriding navigator.geolocation in strict mode.

When I get a moment, I'll look to test how IE handles assignment to a readonly property in strict mode.

[Oliver Hunt]

Rather than disabling strict mode, i'd rather the binding layer just have the concept of a no-op setter

[Geoffrey Garen]

Oliver, what are you proposing exactly?

Are you saying that we should change navigator.geolocation not to be read-only? That has the downside of being incompatible with the Geolocation spec. What's the upside?

[Oliver Hunt]

It sounds like other browsers are treating this as a no-op setter rather than a readonly property, what happens if you try strict mode assignment to them in other browsers?

[Daniel Bates]

(In reply to comment #7)
> It sounds like other browsers are treating this as a no-op setter rather than a readonly property, 

Only Google Chrome for Mac and Opera for Mac have this behavior. And this behavior disagrees with the note following the definition of Simple Assignment in section 11.13.1 of the ECMAScript-262 spec., <http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf> and the 4th bullet in section "The Strict Mode of ECMAScript" (p. 235).

> what happens if you try strict mode assignment to them in other browsers?

I ran the following JavaScript script in Safari, Firefox for Mac, and IE11 to test how they handle assignment to the readonly property navigator.geolocation in strict mode (through an object that extends window.navigator):

(function() { "use strict"; var MyNavigator = function() {}; MyNavigator.prototype = window.navigator; var myNavigator = new MyNavigator(); myNavigator.geolocation = {}; alert("Success"); })()

Here are the results:

Shipping Safari Version 7.0.3 (9537.75.14): JavaScript alert
Safari Version 7.0.3 (9537.75.14) with r169635: JavaScript error - "TypeError: Attempted to assign to readonly property."

Firefox for Mac version 28.0:  JavaScript error - "TypeError: setting property that has only a getter"
Firefox for Mac version 29.0.1:  JavaScript error - "TypeError: setting property that has only a getter"

Google Chrome for Mac version 36.0.1985.49 beta: JavaScript alert

IE11 (11.0.9600.17107): JavaScript error - "Assignment to read-only properties is not allowed in strict mode"

Opera for Mac version 12.15 (1748) with Presto engine: JavaScript alert
Opera for Mac version 22.0.1471.50 with Blink engine: JavaScript alert

[Daniel Bates]

For completeness, navigator.geolocation is defined to be a read only attribute in Geolocation API, <http://www.w3.org/TR/geolocation-API/#geolocation_interface> (W3C Recommendation 24 October 2013).

[Oliver Hunt]

Did we allow them to be overwritten before, or were they simply ignored?

[Daniel Bates]

(In reply to comment #10)
> Did we allow them to be overwritten before, or were they simply ignored?

We allowed such readonly properties to be overwritten in objects that inherited them. With respect to the example code in comment #8, querying myNavigator.geolocation would return an empty dictionary after executing the statement "myNavigator.geolocation = {};".

[Darin Adler]

Comment on attachment 232588 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:337
> +// FIXME: We should find a better place for this function if it becomes useful to callers outside this file.
> +static bool UIKitLinkedOnOrAfter(int version)
> +{
> +    int32_t linkTimeVersion = NSVersionOfLinkTimeLibrary("UIKit");
> +    if (linkTimeVersion == -1) {
> +        // The application was not linked against UIKit so assume most recent JavaScriptCore.
> +        return true;
> +    }
> +    int32_t majorVersion = linkTimeVersion >> 16 & 0x0000FFFF;
> +    return majorVersion >= version;
> +}

Normally we’d use a combination of WebKitVersionChecks.h and RuntimeApplicationChecksIOS.h for these kinds of checks. I understand that it’s handy to have this at the JavaScript level so we can just call out to it, but to fit in with the rest of these kinds of things we could rearrange things so WebKit does the check and makes a call to JavaScriptCore to set this mode.

[Oliver Hunt]

(In reply to comment #11)
> (In reply to comment #10)
> > Did we allow them to be overwritten before, or were they simply ignored?
> 
> We allowed such readonly properties to be overwritten in objects that inherited them. With respect to the example code in comment #8, querying myNavigator.geolocation would return an empty dictionary after executing the statement "myNavigator.geolocation = {};".

What happened to my suggestion of using [Replaceable] to return the old behaviour?  I thought you said that that resolved this problem?

[Daniel Bates]

Created attachment 232707 [details]
Patch and layout tests

Updated patch with Oliver Hunt's suggestion to mark navigator.geolocation as [Replaceable]. The included tests don't pass because the first assignment to an instance attribute called geolocation that shadows navigator.geolocation is ignored (why?). For now, I'm deferring marking this patch for review because of the test failures.

[Daniel Bates]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Did we allow them to be overwritten before, or were they simply ignored?
> > 
> > We allowed such readonly properties to be overwritten in objects that inherited them. With respect to the example code in comment #8, querying myNavigator.geolocation would return an empty dictionary after executing the statement "myNavigator.geolocation = {};".
> 
> What happened to my suggestion of using [Replaceable] to return the old behaviour? 

I posted an updated patch per comment #14.

> I thought you said that that resolved this problem?

As it turns out, the Amazon app issues are resolved as a side effect of making navigator.geolocation [Replaceable]. Specifically, we no longer throw a JavaScript type error in strict mode when assigning a value to an instance attribute that would shadow navigator.geolocation after making navigator.geolocation [Replaceable]. However, the assignment has no effect. That is, subsequently querying for the instance attribute geolocation always returns the Geolocation object (why?).

You can see this issue by applying the patch, attachment 232707 [details], and open the following URL in Safari linked against the built WebKit:
data:text/html,<script>'use strict'; var MyNavigator = function() {}; MyNavigator.prototype = window.navigator; var myNavigator = new MyNavigator(); myNavigator.geolocation = 1; alert(myNavigator.geolocation);</script>

Then the alert message text is "[object Geolocation]" (*). But should be "1". Modify the above URL to assign some arbitrary value to myNavigator.geolocation, say the string literal "dummy", before the statement that assigns 1 to it, such that it reads:

data:text/html,<script>'use strict';  var MyNavigator = function() {}; MyNavigator.prototype = window.navigator; var myNavigator = new MyNavigator(); myNavigator.geolocation = 'dummy'; myNavigator.geolocation = 1; alert(myNavigator.geolocation);</script>

Open this URL. Then the alert message reads "1" as expected.

(*) I noticed that executing a similar script directly in the Web Inspector console will cause the alert message text to read "1" as expected (why?).

[Daniel Bates]

(In reply to comment #14)
> Created an attachment (id=232707) [details]
> Patch and layout tests
> 
> Updated patch with Oliver Hunt's suggestion to mark navigator.geolocation as [Replaceable]. The included tests don't pass because the first assignment to an instance attribute called geolocation that shadows navigator.geolocation is ignored (why?).

As per an in-person conversation with Oliver Hunt today, filed bug #133648 for the ignored first assignment issue with regards to [Replaceable] navigator.geolocation.

[Daniel Bates]

Created attachment 232716 [details]
Patch and layout test

Updated patch as per an in-person conversation with Oliver Hunt today. It's sufficient to test that assigning to an instance attribute that shadows navigator.geolocation doesn't cause a JavaScript type error in strict mode. We'll look to address the idiosyncrasies with making navigator.geolocation [Replaceable] in bug #133648.

[Daniel Bates]

Committed r169710: <http://trac.webkit.org/changeset/169710>