refactor($browser): wrap Get(history.state) in try/catch #10369

caitp · 2014-12-08T14:17:25Z

Reportedly, MSIE can throw under certain conditions when fetching this attribute.

Will confirm before landing.

Reportedly, MSIE can throw under certain conditions when fetching this attribute. Will confirm before landing. Fixes angular#10367

googlebot · 2014-12-08T14:17:26Z

CLAs look good, thanks!

pkozlowski-opensource · 2014-12-08T14:25:39Z

It would be totally awesome to try to narrow down "certain conditions" IMO

caitp · 2014-12-08T14:29:11Z

Yeah --- I'll probably bug mr. terlson about it later (but I feel bad about doing that, which is why I really need to find more IE developers to bug with this stuff :>) maybe there's something on their issue tracker

caitp · 2014-12-08T14:33:26Z

FWIW I wasn't able to reproduce in SL, but that doesn't mean much --- the IE feedback site is too hard to find anything on, so it's gonna come down to asking I think

gkalpak · 2014-12-08T17:10:10Z

src/ng/browser.js

@@ -235,10 +235,22 @@ function Browser(window, document, $log, $sniffer) {

  // This variable should be used *only* inside the cacheState function.
  var lastCachedState = null;
+  function getCurrentState() {
+    try {
+      var state = history.state;


Curious: Why not use window ?

we cache history anyways, might as well use it

shahata · 2015-01-25T07:04:50Z

@caitp any progress with this? Apparently we are experiencing this issue in one of our apps as well.

caitp · 2015-01-25T17:18:33Z

try the patch and see if it fixes it, i have no idea how we test this since nobody has produced a repro scenario

shahata · 2015-01-25T20:13:36Z

Yeah, I found out a couple of days ago that we've been using this patch for a while now in some project. @donataswix any chance you can post a reproduce of this issue?

caitp · 2015-01-25T20:41:58Z

Yeah, I found out a couple of days ago that we've been using this patch for a while now in some project.

does that mean the patch solved some issue, or it's still causing problems?

shahata · 2015-01-25T20:50:51Z

Yes, this patch solved the issue. Hopefully @donataswix will be able to add more details tomorrow.

donataswix · 2015-01-26T08:00:34Z

The patch solved the issue, I'll try to make reproducible example later today or at least figure out some more details.

petebacondarwin · 2015-01-27T11:49:53Z

@donataswix - that would be extremely helpful. Thanks

donataswix · 2015-01-27T12:22:34Z

For better or worse I can no longer reproduce this no matter what I try. So even though that patch helped at the time, it's no longer necessary for us. Although, I'm not sure I want to remove it from our build...

If anyone's interested our setup has dynamically created iframe where our app is loaded. There is a lot of code that runs before ours, and it seems that that code was responsible for creating preconditions for bug to appear.

petebacondarwin · 2015-01-27T14:03:45Z

src/ng/browser.js

+  function getCurrentState() {
+    try {
+      var state = history.state;
+      if (state === void 0) {


Perhaps we should change isDefined() to use this method of testing for undefined and use it here?

Since history.state is either going to be a valid object, undefined or null, or it is going to throw an error we can simply do:

function getCurrentState() { try { return history.state || null; } catch(e) { return null; } }

It's not going to be either a valid object, undefined or null, eg https://dxr.mozilla.org/mozilla-central/source/dom/webidl/History.webidl#18

could be "false", could be "0", could be the empty string, etc

technically, it can legally be undefined, too... so hmm

Looks like we are pretending that it will never be undefined : https://github.com/angular/angular.js/blob/master/src/ng/browser.js#L159-L164

petebacondarwin · 2015-02-09T13:02:44Z

Given that whatever we do here it is not going to be correct, we should keep the behaviour as close to what it is currently, so can we simply go with the following?

function getCurrentState() {
  try {
    return history.state;
  } catch (e) {
    // MSIE can reportedly throw when there is no state (UNCONFIRMED).
    return null;
  }
}

function cacheState() {
  cachedState = getCurrentState();
  cachedState = isUndefined(cachedState) ? null : cachedState;
}

Is that OK with you @caitp ?

dbabaioff · 2015-02-09T13:04:25Z

We also are experiencing this issue. We tried the patch and didn't work (it didn't throw an exception). To solve we did like this (when we load the page):

window.history._state = null;

Object.defineProperty(window.history, 'state', {
    get: function() { return window.history._state; },
    set: function(state) { window.history._state = state; }
});

petebacondarwin · 2015-02-09T13:13:20Z

@dbabaioff - when you say it didn't throw an exception with this patch, do you mean that you get the error message without an error being thrown? Are you able to create a reproduction of this that we could add to our unit tests?

dbabaioff · 2015-02-09T13:15:00Z

@petebacondarwin - yes - I get the error message ("SCRIPT16389: Unspecified error .") without an error being throw. I'll try to create a reproduction of this.

petebacondarwin · 2015-02-27T21:22:31Z

@dbabaioff - did you ever manage to create a reproduction of this without an error being thrown?

Reportedly, MSIE can throw under certain conditions when fetching this attribute. We don't have a reliable reproduction for this but it doesn't do any real harm to wrap access to this variable in a try-catch block. Fixes #10367 Closes #10369

Reportedly, MSIE can throw under certain conditions when fetching this attribute. We don't have a reliable reproduction for this but it doesn't do any real harm to wrap access to this variable in a try-catch block. Fixes angular#10367 Closes angular#10369

filipposarzana · 2016-02-05T17:50:25Z

@dbabaioff yes! it worked for me like a charm! Thanks!!!

refactor($browser): wrap Get(history.state) in try/catch

0d7fb9c

Reportedly, MSIE can throw under certain conditions when fetching this attribute. Will confirm before landing. Fixes angular#10367

googlebot added the cla: yes label Dec 8, 2014

gkalpak reviewed Dec 8, 2014
View reviewed changes

caitp added component: misc core type: refactor frequency: low labels Dec 8, 2014

caitp added this to the Backlog milestone Dec 8, 2014

caitp added the browser: IE10 label Dec 8, 2014

petebacondarwin modified the milestones: 1.4.0-beta.3 / 1.3.12, Backlog Jan 27, 2015

petebacondarwin self-assigned this Jan 27, 2015

petebacondarwin reviewed Jan 27, 2015
View reviewed changes

petebacondarwin assigned caitp and unassigned petebacondarwin Jan 29, 2015

petebacondarwin modified the milestones: 1.4.0-beta.3 / 1.3.12, 1.4.0-beta.4 / 1.3.13 Feb 3, 2015

petebacondarwin modified the milestones: 1.4.0-beta.4 / 1.3.13, 1.4.0-beta.5 / 1.3.14 Feb 9, 2015

petebacondarwin modified the milestones: 1.4.0-beta.5 / 1.3.14, 1.4.0-beta.6 / 1.3.15 Feb 24, 2015

caitp closed this in 3b8163b Mar 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor($browser): wrap Get(history.state) in try/catch #10369

refactor($browser): wrap Get(history.state) in try/catch #10369

caitp commented Dec 8, 2014

googlebot commented Dec 8, 2014

pkozlowski-opensource commented Dec 8, 2014

caitp commented Dec 8, 2014

caitp commented Dec 8, 2014

gkalpak Dec 8, 2014

caitp Dec 8, 2014

shahata commented Jan 25, 2015

caitp commented Jan 25, 2015

shahata commented Jan 25, 2015

caitp commented Jan 25, 2015

shahata commented Jan 25, 2015

donataswix commented Jan 26, 2015

petebacondarwin commented Jan 27, 2015

donataswix commented Jan 27, 2015

petebacondarwin Jan 27, 2015

petebacondarwin Jan 30, 2015

caitp Jan 30, 2015

caitp Jan 30, 2015

caitp Jan 30, 2015

petebacondarwin Jan 30, 2015

petebacondarwin commented Feb 9, 2015

dbabaioff commented Feb 9, 2015

petebacondarwin commented Feb 9, 2015

dbabaioff commented Feb 9, 2015

petebacondarwin commented Feb 27, 2015

filipposarzana commented Feb 5, 2016

refactor($browser): wrap Get(history.state) in try/catch #10369

refactor($browser): wrap Get(history.state) in try/catch #10369

Conversation

caitp commented Dec 8, 2014

googlebot commented Dec 8, 2014

pkozlowski-opensource commented Dec 8, 2014

caitp commented Dec 8, 2014

caitp commented Dec 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahata commented Jan 25, 2015

caitp commented Jan 25, 2015

shahata commented Jan 25, 2015

caitp commented Jan 25, 2015

shahata commented Jan 25, 2015

donataswix commented Jan 26, 2015

petebacondarwin commented Jan 27, 2015

donataswix commented Jan 27, 2015

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 Feb 9, 2015

dbabaioff commented Feb 9, 2015

petebacondarwin commented Feb 9, 2015

dbabaioff commented Feb 9, 2015

petebacondarwin commented Feb 27, 2015

filipposarzana commented Feb 5, 2016