Bug 169970

Summary: JavaScript is not correctly connecting to rest api in iOS 10.3 beta
Product: WebKit Reporter: paladox <thomasmulhall410>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Blocker CC: ap, beidson, jer.noble, simon.fraser, webkit-bug-importer, youennf
Priority: P1 Keywords: InRadar, Regression
Version: Safari Technology Preview   
Hardware: iPhone / iPad   
OS: iOS 10   

Description paladox 2017-03-22 13:47:35 PDT
Hi, since upgrading to iOS 10.3 beta loading websites like https://gerrit-review.googlesource.com?polygerrit=1 is no loading.

https://github.com/GerritCodeReview/gerrit/tree/master/polygerrit-ui

Also testing in safari technology preview results in the same issue.

I am unsure if it is a bug that was introduced by apple or if it is a bug in webkit. So I'm filling it here. I filled a report here https://bugs.chromium.org/p/gerrit/issues/detail?id=5715 for gerrit.

Sorry that i doint have much more information. I tried for a week to get more info but i carn't as the developer tools for js errors isn't telling me much.

iOS 10.2 is un affected.
Comment 1 paladox 2017-03-22 13:51:08 PDT
Oh wait it seems to work on gerri-review with the safari technology preview so it seems it is just iOS 10.3.
Comment 2 paladox 2017-03-22 14:15:30 PDT
Testing locally and this fails on safari technology preview but not on gerrit-review site. So maybe cross origin?
Comment 3 paladox 2017-03-22 14:16:49 PDT
(In reply to paladox from comment #2)
> Testing locally and this fails on safari technology preview but not on
> gerrit-review site. So maybe cross origin?

I should i press the clear cache button. It works locally now with safari technology preview but not on iOS 10.3
Comment 4 Simon Fraser (smfr) 2017-03-22 15:10:28 PDT
Does the Web Inspector consoles show any errors?
Comment 5 paladox 2017-03-23 01:29:19 PDT
Hi, the only error it shows is this

console.error(e.detail.error.message);
             ^
Is showing as undefined not an object.

But removing that part stops the undefined not an object error in the console but it fix the problem.
Comment 6 paladox 2017-03-23 01:30:34 PDT
^^ sorry where I say it fixed the problem, it dosent fix the problem.
Comment 7 paladox 2017-03-24 06:16:21 PDT
Bum.

I am not 100% correct its about fetching json but what ever the problem is it is not correctly connecting to the rest api on gerrit using polygerrit. I doint see anything in the console to explain this problem. Polygerrit throws the network-error error internally to throw Server Unavilable.

This works on iOS 10.2 but breaks on iOS 10.3 beta.
Comment 8 paladox 2017-03-24 06:16:45 PDT
bum = bump.
Comment 9 Simon Fraser (smfr) 2017-03-24 10:20:00 PDT
Does the same problem occur on Mac, with built-in Safari or Safari Tech Preview?
Comment 10 paladox 2017-03-24 11:19:58 PDT
Hi nope doesn't happen on safari or the safari technology preview. Im running mac OS X 10.12.3.
Comment 11 paladox 2017-03-24 15:49:18 PDT
It looks like polygerrit uses https://github.com/github/fetch/

Example code

   fetchJSON: function(url, opt_errFn, opt_cancelCondition, opt_params,
        opt_opts) {
      opt_opts = opt_opts || {};
      var fetchOptions = {
        credentials: 'same-origin',
        headers: opt_opts.headers,
      };

      var urlWithParams = this._urlWithParams(url, opt_params);
      return fetch(urlWithParams, fetchOptions).then(function(response) {
        if (opt_cancelCondition && opt_cancelCondition()) {
          response.body.cancel();
          return;
        }

        if (!response.ok) {
          if (opt_errFn) {
            opt_errFn.call(null, response);
            return;
          }
          this.fire('server-error', {response: response});
          return;
        }

        return this.getResponseObject(response);
      }.bind(this)).catch(function(err) {
        if (opt_errFn) {
          opt_errFn.call(null, null, err);
        } else {
          this.fire('network-error', {error: err});
          throw err;
        }
        throw err;
      }.bind(this));
    },
Comment 12 paladox 2017-03-24 16:09:39 PDT
It seems safari 10.1 includes native fetch api now https://developer.apple.com/library/prerelease/content/releasenotes/General/WhatsNewInSafari/Articles/Safari_10_1.html that could be breaking this.
Comment 13 paladox 2017-03-24 16:22:53 PDT
Maybe safari 10.1 is missing the Fetch API: streaming response body api?

According to https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API it doesn't have that api in safari 10.1. I think polygerrit uses that api.
Comment 14 paladox 2017-03-24 16:35:44 PDT
Nope polygerrit doesn't seem to use streaming response body api as the GitHub fetch library doesn't use it. So theres a bug some where in webkit with fetch.
Comment 15 paladox 2017-03-25 06:17:40 PDT
It seems the response api in fetch is not in safari 10.1 https://developer.mozilla.org/en-US/docs/Web/API/Response and polygerrit uses that https://github.com/gerrit-review/gerrit/search?l=JavaScript&q=response.ok&type=&utf8=%E2%9C%93
Comment 17 youenn fablet 2017-03-25 23:11:44 PDT
Hi paladox,

Are you seeing this stack trace (to get it, open web inspector and place a breakpoint where the error is raised)?

anonymous@[native code]
fillFetchHeaders@[native code]
initializeFetchRequest@[native code]
[native code]
fetch@[native code]
fetchJSON@https://cdn.googlesource.com/polygerrit_ui/304.0/elements/gr-app.js:9878:19
_fetchSharedCacheURL@https://cdn.googlesource.com/polygerrit_ui/304.0/elements/gr-app.js:10145:54
attached@https://cdn.googlesource.com/polygerrit_ui/304.0/elements/gr-app.js:40761:31
Comment 18 youenn fablet 2017-03-25 23:26:32 PDT
If this is the stack trace you are seeing, the issue might be https://bugs.webkit.org/show_bug.cgi?id=168043.
You might want to verify this by checking the fetch options passed and, if you can, modify it to not have any option with {headers: undefined}.
Comment 19 paladox 2017-03-26 06:44:26 PDT
(In reply to youenn fablet from comment #17)
> Hi paladox,
> 
> Are you seeing this stack trace (to get it, open web inspector and place a
> breakpoint where the error is raised)?
> 
> anonymous@[native code]
> fillFetchHeaders@[native code]
> initializeFetchRequest@[native code]
> [native code]
> fetch@[native code]
> fetchJSON@https://cdn.googlesource.com/polygerrit_ui/304.0/elements/gr-app.
> js:9878:19
> _fetchSharedCacheURL@https://cdn.googlesource.com/polygerrit_ui/304.0/
> elements/gr-app.js:10145:54
> attached@https://cdn.googlesource.com/polygerrit_ui/304.0/elements/gr-app.js:
> 40761:31

Hi, nope i doint see any trace back except from it thinks e.detail.error.message is undefined.

It seems you got the traceback.

I ran this

    var result = fetch('https://api.github.com')
    result.then(function(response) {
      console.error('response', response)
      console.error('header', response.headers.get('Content-Type'))
      return response.text()
    }).then(function(text) {
      console.error('got text', text)
    }).catch(function(ex) {
      console.error('failed', ex)
    })

through tried this on jsfiddle on iOS 10.3 and got

iOS 10.3

{
  \"current_user_url\": \"https://api.github.com/user\",
  \"current_user_authorizations_html_url\": \"https://github.com/settings/connections/applications{/client_id}\",
  \"authorizations_url\": \"https://api.github.com/authorizations\",
  \"code_search_url\": \"https://api.github.com/search/code?q={query}{&page,per_page,sort,order}\",
  \"commit_search_url\": \"https://api.github.com/search/commits?q={query}{&page,per_page,sort,order}\",
  \"emails_url\": \"https://api.github.com/user/emails\",
  \"emojis_url\": \"https://api.github.com/emojis\",
  \"events_url\": \"https://api.github.com/events\",
  \"feeds_url\": \"https://api.github.com/feeds\",
  \"followers_url\": \"https://api.github.com/user/followers\",
  \"following_url\": \"https://api.github.com/user/following{/target}\",
  \"gists_url\": \"https://api.github.com/gists{/gist_id}\",
  \"hub_url\": \"https://api.github.com/hub\",
  \"issue_search_url\": \"https://api.github.com/search/issues?q={query}{&page,per_page,sort,order}\",
  \"issues_url\": \"https://api.github.com/issues\",
  \"keys_url\": \"https://api.github.com/user/keys\",
  \"notifications_url\": \"https://api.github.com/notifications\",
  \"organization_repositories_url\": \"https://api.github.com/orgs/{org}/repos{?type,page,per_page,sort}\",
  \"organization_url\": \"https://api.github.com/orgs/{org}\",
  \"public_gists_url\": \"https://api.github.com/gists/public\",
  \"rate_limit_url\": \"https://api.github.com/rate_limit\",
  \"repository_url\": \"https://api.github.com/repos/{owner}/{repo}\",
  \"repository_search_url\": \"https://api.github.com/search/repositories?q={query}{&page,per_page,sort,order}\",
  \"current_user_repositories_url\": \"https://api.github.com/user/repos{?type,page,per_page,sort}\",
  \"starred_url\": \"https://api.github.com/user/starred{/owner}{/repo}\",
  \"starred_gists_url\": \"https://api.github.com/gists/starred\",
  \"team_url\": \"https://api.github.com/teams\",
  \"user_url\": \"https://api.github.com/users/{user}\",
  \"user_organizations_url\": \"https://api.github.com/user/orgs\",
  \"user_repositories_url\": \"https://api.github.com/users/{user}/repos{?type,page,per_page,sort}\",
  \"user_search_url\": \"https://api.github.com/search/users?q={query}{&page,per_page,sort,order}\"
}
Comment 20 paladox 2017-03-26 06:45:01 PDT
Is there any script i could try to test weather the header being undefined is causing this please?
Comment 21 paladox 2017-03-26 06:55:16 PDT
Doing something like



function _urlWithParams(url) {
      return url;
    }
    var urlWithParams = _urlWithParams('https://gerrit-review.googlesource.com/config/server/version');
var fetchOptions = {
        //headers: '',
        credentials: 'same-origin',
        mode: 'no-cors',
        headers: {'content-type': 'application/json'},
      };
  fetch(urlWithParams, fetchOptions)
    .then(function(response) {
      console.error('response', response)
      console.error('header', response.headers.get('Content-Type'))
      return response.text().then(function(text) {
        var result;
        try {
          result = JSON.parse(text.substring(JSON_PREFIX.length));
        } catch (_) {
          result = null;
        }
        console.error('got text', text)
        return result;
      });
    }).catch(function(ex) {
      console.error('failed', ex)
    })

fails. I may have done it wrong but not sure.
Comment 22 paladox 2017-03-26 06:55:45 PDT
It doesn't respond with a url

response Response {type: "opaque", url: "", redirected: false, status: 0, ok: falseā€¦}
Comment 23 paladox 2017-03-26 07:42:50 PDT
The traceback is leading me to this line


    fetchJSON: function(url, opt_errFn, opt_cancelCondition, opt_params,
        opt_opts) {
      opt_opts = opt_opts || {};
      var fetchOptions = {
        credentials: 'same-origin',
        headers: opt_opts.headers,
      };

      var urlWithParams = this._urlWithParams(url, opt_params);
      return fetch(urlWithParams, fetchOptions).then(function(response) {
        if (opt_cancelCondition && opt_cancelCondition()) {
          response.body.cancel();
          return;
        }

        if (!response.ok) {
          if (opt_errFn) {
            opt_errFn.call(null, response);
            return;
          }
          this.fire('server-error', {response: response});
          return;
        }

        return this.getResponseObject(response);
      }.bind(this)).catch(function(err) {
        if (opt_errFn) {
          opt_errFn.call(null, null, err);
        } else {
          this.fire('network-error', {error: err});
          throw err;
        }
        throw err;
      }.bind(this));
    },
Comment 24 paladox 2017-03-26 08:13:03 PDT
I added console.error to fetchJson and i got some logs in the console if i used safari on the mac or the safari technology preview. But i got none on iOS 10.3 safari.
Comment 25 youenn fablet 2017-03-26 09:02:09 PDT
Can you update fetchOptions so that its 'headers' value has a real value or does not exist at all. Having {headers: undefined} might be the issue.
Also you can get the stack trace doing something like e.detail.error.stack
Comment 26 paladox 2017-03-26 09:17:42 PDT
I found safari technology preview 23 download. I tried that and polygerrit docent work in that release. It seems indeed its the undefined header thing. Could your fix be back ported into safari 10.1 for iOS 10.3 and presumingly mac 10.12.4 is using it, please?
Comment 27 paladox 2017-03-26 09:19:02 PDT
(In reply to youenn fablet from comment #25)
> Can you update fetchOptions so that its 'headers' value has a real value or
> does not exist at all. Having {headers: undefined} might be the issue.
> Also you can get the stack trace doing something like e.detail.error.stack

I doint know how to get the header to be anything else. I tried crossing out headers but that still didn't work.
Comment 28 paladox 2017-03-26 09:23:37 PDT
Removing headers: seems to work on the safari technology preview 23.
Comment 29 paladox 2017-03-26 09:25:38 PDT
It also works on ios when removing the headers part.
Comment 30 paladox 2017-03-26 13:01:52 PDT
Removing the headers: seems to break refreshing a diff. So could the fix you did be back ported to 10.1 please?
Comment 31 youenn fablet 2017-03-26 14:47:32 PDT
Can you try setting headers only if not undefined?
Something like:
fetchOptions ={}
if (opt_opts.headers)
    fetchOptions.headers = opt_opts.headers
Comment 32 paladox 2017-03-26 15:38:31 PDT
I did 

      if (opt_opts.headers !== undefined) {
      var fetchOptions = {
        credentials: 'same-origin',
        headers: opt_opts.headers,
      };
      } else {
      var fetchOptions = {
        credentials: 'same-origin',
       // headers: opt_opts.headers,
      };
      }

which is the same as if(opt_opts.headers).

But still doesn't work in reloading the diff page. For example going to http://gerrit-new.wmflabs.org/r/c/2/62/tests/fixtures/layout-cloner.yaml and then reloading that downloads a json file (the fix i did for headers is not on that site). It should not be downloading json files when refreshing the page. Which i presume thats why headers: is there to fix that.
Comment 33 paladox 2017-03-26 15:40:01 PDT
the diff loads if you click on the file to view the diff http://gerrit-new.wmflabs.org/r/c/2/

for example ^^ going to that and clicking on the file works. But refreshing it downloads a json file instead of reloading the page. Same for when clicking the next and prev buttons on the diff.
Comment 34 paladox 2017-03-27 10:33:41 PDT
@youenn fablet hi, is there a way i can remove the headers: part only on the browser versions that the problem exist. Like safari 10.1 please? Since iOS 10.3 and mac os 10.12.4 have rolled out now.
Comment 35 youenn fablet 2017-03-27 11:27:53 PDT
(In reply to paladox from comment #32)
> I did 
> 
>       if (opt_opts.headers !== undefined) {
>       var fetchOptions = {
>         credentials: 'same-origin',
>         headers: opt_opts.headers,
>       };
>       } else {
>       var fetchOptions = {
>         credentials: 'same-origin',
>        // headers: opt_opts.headers,
>       };
>       }
> 
> which is the same as if(opt_opts.headers).
> 
> But still doesn't work in reloading the diff page. For example going to
> http://gerrit-new.wmflabs.org/r/c/2/62/tests/fixtures/layout-cloner.yaml and
> then reloading that downloads a json file (the fix i did for headers is not
> on that site). It should not be downloading json files when refreshing the
> page. Which i presume thats why headers: is there to fix that.

If it is still not working, there might be another underlying bug somewhere.
Can you try checking the headers purpose and see whether there is something different with the patch in network inspector?
Time permitting, I can also look at it if I got access to a web site with this patch on.
Comment 36 Radar WebKit Bug Importer 2017-03-27 11:28:19 PDT
<rdar://problem/31278173>
Comment 37 paladox 2017-03-27 12:09:09 PDT
Looking into it more. I'm thinking it's https://gerrit-review.googlesource.com/#/c/100912/ . As I merged something into that patch and also updated it which may have lead to the problem with refreshing diffs. As leading the headers: as it is works in chrome but refreshing a diff fails too.

Must be the way I updated https://gerrit-review.googlesource.com/#/c/100912/
Comment 38 youenn fablet 2017-03-28 10:44:19 PDT
paradox, can we close the bug here as duplicate of https://bugs.webkit.org/show_bug.cgi?id=168043, or is there some additional issue?
Comment 39 paladox 2017-03-28 17:27:38 PDT
Yep we can close this as duplicate of https://bugs.webkit.org/show_bug.cgi?id=168043
Comment 40 Alexey Proskuryakov 2017-03-28 17:32:23 PDT

*** This bug has been marked as a duplicate of bug 168043 ***