fix($sniffer): fix history sniffing in Chrome Packaged Apps #13945

gkalpak · 2016-02-04T11:06:48Z

Although window.history is present in the context of Chrome Packaged Apps, it is not allowed to
access window.history.pushState or window.history.state, resulting in errors when trying to
"sniff" history support.
This commit fixes it by detecting a Chrome Packaged App (through the presence of
window.chrome.app.runtime). Note that window.chrome.app is present in the context of "normal"
webpages as well, but it doesn't have the runtime property, which is only available to packaged
apps (e.g. see https://developer.chrome.com/apps/api_index).

Fixes #11932

The first commit is just some cleanup (that makes the diff look worse than it actually is).
Included it separately, so the fix is easier to review - could be squashed upon merging.

Although `window.history` is present in the context of Chrome Packaged Apps, it is not allowed to access `window.history.pushState` or `window.history.state`, resulting in errors when trying to "sniff" history support. This commit fixes it by detecting a Chrome Packaged App (through the presence of `window.chrome.app.runtime`). Note that `window.chrome.app` is present in the context of "normal" webpages as well, but it doesn't have the `runtime` property, which is only available to packaged apps (e.g. see https://developer.chrome.com/apps/api_index). Fixes angular#11932

Narretz · 2016-02-04T12:04:28Z

src/ng/sniffer.js

@@ -17,6 +17,8 @@
 function $SnifferProvider() {
  this.$get = ['$window', '$document', function($window, $document) {
    var eventSupport = {},
+        isChromePackagedApp = $window.chrome && $window.chrome.app && $window.chrome.app.runtime,
+        hasHistoryPushState = $window.history && $window.history.pushState,


Just an idea, but could we use 'pushstate' in $window.history?

Yes, we can. But what is the benefit ? We want to return false in Chrome Packaged Apps (and 'pushState' in window.history returns true, although we are not allowed to access history.pushState or histroy.state).

(BTW, this fix is not correct, I need to update it.)

Ah, okay, I see

gkalpak · 2016-02-04T13:37:33Z

I pushed a 3rd commit that prevents logging exceptions in the console. PTAL

petebacondarwin · 2016-03-21T19:08:16Z

src/ng/sniffer.js

@@ -61,7 +65,7 @@ function $SnifferProvider() {
      // so let's not use the history API also
      // We are purposefully using `!(android < 4)` to cover the case when `android` is undefined
      // jshint -W018
-      history: !!($window.history && $window.history.pushState && !(android < 4) && !boxee),
+      history: !!(hasHistoryPushState && !(android < 4) && !boxee),


why move the $window.history.... checks out of this expression? Is Chrome more special that android < 4 or boxee?

I would expect it is clearer to simply change this to:

!!(!isChromePackagedApp && $window.history && $window.history.pushState && !(android < 4) && !boxee),

Is Chrome more special that android < 4 or boxee

Yeah, Chrome is actually very close to my heart 😛

I put it this way for readability (avoiding !!(! and hitting the 100 chars per line limit, thus having to wrap).
I'm fine having it either way.

Plus, if I have isChromePackagedApp and hasHistoryPushState close together, it's easier for reference wrt to the comment explaining why Chrome Packaged Apps need special handling when checking for the existence of history.pushState.

OK, how about:

// Chrome Packaged Apps are not allowed to access `history.pushState`. They can be detected by // the presence of `chrome.app.runtime` (see https://developer.chrome.com/apps/api_index) isChromePackagedApp = $window.chrome && $window.chrome.app && $window.chrome.app.runtime, hasHistoryPushState = $window.history && $window.history.pushState,

then later:

history: !isChromePackagedApp && !!hasHistoryPushState && !(android < 4) && !boxee,

The thing is that we shouldn't even try to touch history.pushState in packaged apps (or there'll be errors).
That's why we need the !isChromePackagedApp short-circuiting at the beginning of hasHistoryPushState.

petebacondarwin · 2016-03-21T19:11:39Z

A few minor comments but generally looks good to me.

petebacondarwin · 2016-03-22T07:16:17Z

LGTM - squash and merge

Although `window.history` is present in the context of Chrome Packaged Apps, it is not allowed to access `window.history.pushState` or `window.history.state`, resulting in errors when trying to "sniff" history support. This commit fixes it by detecting a Chrome Packaged App (through the presence of `window.chrome.app.runtime`). Note that `window.chrome.app` is present in the context of "normal" webpages as well, but it doesn't have the `runtime` property, which is only available to packaged apps (e.g. see https://developer.chrome.com/apps/api_index). (It also also contains some style changes for making the structure and layout of `$sniffer` tests more consistent.) Fixes #11932 Closes #13945

Check in $sniffer if operating in a sandboxed Chrome app, which does not have access to chrome.app.runtime like other apps, but also does not have access to history.pushState. If inside a sandboxed Chrome app, do not try and use history.pushState, else an error will occur. See angular#11932 and angular#13945 for previous work.

…aged Apps While sandboxed Chrome Packaged Apps (CPAs) have the same restrictions wrt accessing `history.pushState` as "normal" CPAs, they can't be detected in the same way, as they do not have access to the same APIs. Previously, due to their differences from normal CPAs, `$sniffer` would fail to detect sandboxed CPAs and incorrectly assume `history.pushState` is available (which resulted in an error being thrown). This commit fixes the detection of sandboxed CPAs in `$sniffer`. See #11932 and #13945 for previous work. Closes #15021

…aged Apps While sandboxed Chrome Packaged Apps (CPAs) have the same restrictions wrt accessing `history.pushState` as "normal" CPAs, they can't be detected in the same way, as they do not have access to the same APIs. Previously, due to their differences from normal CPAs, `$sniffer` would fail to detect sandboxed CPAs and incorrectly assume `history.pushState` is available (which resulted in an error being thrown). This commit fixes the detection of sandboxed CPAs in `$sniffer`. See angular#11932 and angular#13945 for previous work. Closes angular#15021

gkalpak added 2 commits February 4, 2016 12:53

test($sniffer): make the structure and layout of tests more consistent

566b31c

googlebot added the cla: yes label Feb 4, 2016

gkalpak added this to the Backlog milestone Feb 4, 2016

gkalpak added type: bug frequency: low needs: review component: misc core severity: broken expected use browser: chrome labels Feb 4, 2016

gkalpak mentioned this pull request Feb 4, 2016

history.pushState is not available in packaged apps. #11932

Closed

Narretz reviewed Feb 4, 2016
View reviewed changes

fixup 1

6cab4ad

gkalpak force-pushed the fix-sniffer-history-for-chrome-packaged-apps branch from 6d84ed5 to 6cab4ad Compare February 4, 2016 13:36

fixup 2

047587d

petebacondarwin reviewed Mar 21, 2016
View reviewed changes

petebacondarwin added needs: merging and removed needs: review labels Mar 22, 2016

gkalpak closed this in 9f5526f Mar 22, 2016

gkalpak deleted the fix-sniffer-history-for-chrome-packaged-apps branch March 27, 2016 17:49

Parent5446 mentioned this pull request Aug 12, 2016

fix($sniffer): don't use history.pushState for sandboxed Chrome apps #15021

Closed

3 tasks

gkalpak mentioned this pull request Jun 29, 2017

Firefox WebExtensions misdetected as Chrome Packaged Apps #16068

Open

3 tasks

gkalpak mentioned this pull request Jan 24, 2019

feat(docs-infra): saves the scroll position before the change of loca… angular/angular#28037

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($sniffer): fix history sniffing in Chrome Packaged Apps #13945

fix($sniffer): fix history sniffing in Chrome Packaged Apps #13945

gkalpak commented Feb 4, 2016

Narretz Feb 4, 2016

gkalpak Feb 4, 2016

Narretz Feb 4, 2016

gkalpak commented Feb 4, 2016

petebacondarwin Mar 21, 2016

gkalpak Mar 21, 2016

gkalpak Mar 21, 2016

petebacondarwin Mar 21, 2016

gkalpak Mar 21, 2016

petebacondarwin Mar 22, 2016

petebacondarwin commented Mar 21, 2016

petebacondarwin commented Mar 22, 2016

fix($sniffer): fix history sniffing in Chrome Packaged Apps #13945

fix($sniffer): fix history sniffing in Chrome Packaged Apps #13945

Conversation

gkalpak commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Mar 21, 2016

petebacondarwin commented Mar 22, 2016