Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($browser): colon in url causes infinite digest in FF #8734

Closed
wants to merge 2 commits into from
Closed

fix($browser): colon in url causes infinite digest in FF #8734

wants to merge 2 commits into from

Conversation

mackenco
Copy link

Bug found when an angular app is running in an iframe + has a colon in the query string in Firefox. This stops angular from working until the page is refreshed, errors are:

Error: [$rootScope:infdig] 10 $digest() iterations reached. Aborting!
Watchers fired in the last 5 iterations: [["fn: $locationWatch; newVal: 7; oldVal: 6"],["fn: $locationWatch; newVal: 8; oldVal: 7"],["fn: $locationWatch; newVal: 9; oldVal: 8"],["fn: $locationWatch; newVal: 10; oldVal: 9"],["fn: $locationWatch; newVal: 11; oldVal: 10"]]

(fixed with the addition to line 178)
and

Exception { message: "", result: 2147500037, name: "NS_ERROR_FAILURE"}

(fixed with the addition to line 125).

What is happening is that Firefox's location.href encodes colons (after string position seven) to %3A. This doesn't match with other encoding and the app gets stuck. For example, we were passing in an absolute url through the query and getting issues; it also happened with the port on localhost. Very similar to: #920.
To reproduce, just run a small app locally in Firefox (I was able to recreate with angular phonecat) in an iframe.

<html>    
    <body>
        <iframe src="http://localhost:8000/app/index.html#/?url=https:www.google.com>
    </body>
</html>

@mackenco
fix($browser): colin in url causes infinite digest in FF
df2b001 
FF encodes colons after string position seven in urls, this causes .url() != currentUrl.absUrl(), which leads to infinite digest.

The workaround is to correct this in $browser and decode colons in urls.
@caitp
Copy link
Contributor

caitp commented Aug 22, 2014

missing a test case

@mackenco
Copy link
Author

@caitp, added a test

@caitp caitp modified the milestones: Backlog, 1.3.0 Aug 22, 2014
@IgorMinar
Copy link
Contributor

I can't reproduce this one with FF31: http://plnkr.co/edit/Yh2uKjzcPCGFySC8NddA?p=preview

can you please adjust the plunker to show the buggy behavior?

thanks!

@caitp
Copy link
Contributor

caitp commented Aug 23, 2014

I don't think that's a real reproduction because we aren't doing anything to rewrite the URL in that script Igor, I don't see angular being used in either the iframe or the main page there.

@caitp
Copy link
Contributor

caitp commented Aug 23, 2014

I've modified Igor's plunker to use src="http://angular.github.io/angular-phonecat/step-9/app/index.html#/?url=https:www.google.com" instead, and I still can't see the issue with FF31. :(

@caitp
Copy link
Contributor

caitp commented Aug 23, 2014

should be noted: the test case fails in other browsers without the fix applied, too

caitp
caitp reviewed Aug 23, 2014
View reviewed changes
test/ng/browserSpecs.js
@@ -466,6 +466,11 @@ describe('browser', function() {
expect(browser.url()).toBe("http://ff-bug/?single'quote");
});

it('should decode colons to work around FF bug', function() {
fakeWindow.location.href = "http://ff-bug/?https%3Awww.google.com";
expect(browser.url()).toBe("http://ff-bug/?https:www.google.com");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is also not verifying that you're preventing an infinite digest error, which is what this bug is about. We need to be able to reproduce the infdig condition

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried to mimic the described FF behavior without angular. The test
looks fishy to me. It just test that we rewrite the url without actually
reproducing the browser issue. That would be fine if at least we could
repro the browser issue in isolation but we can't.
On Aug 22, 2014 5:50 PM, "Caitlin Potter" [email protected] wrote:

In test/ng/browserSpecs.js:

@@ -466,6 +466,11 @@ describe('browser', function() {
expect(browser.url()).toBe("http://ff-bug/?single'quote");
});

  • it('should decode colons to work around FF bug', function() {
  •  fakeWindow.location.href = "http://ff-bug/?https%3Awww.google.com";

  •  expect(browser.url()).toBe("http://ff-bug/?https:www.google.com");

This test is also not verifying that you're preventing an infinite digest
error, which is what this bug is about

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/8734/files#r16627434.

@IgorMinar
Copy link
Contributor

@mackenco can you please provide more info on how to reproduce this issue. Otherwise we'll close this PR. thanks!

@IgorMinar IgorMinar modified the milestones: 1.3.0, 1.3.0-rc.1 Aug 25, 2014
@mackenco
Copy link
Author

Hey guys - I've tried a handful of different things, but unfortunately haven't been able to get any of the tests to recreate the issue. Looks like it may just be something that was happening in my app's specific environment + FF. Closest I could get was the infdig issue happening when running the phonecat app on localhost in an iframe in FF. A bummer for me, but you can probably go ahead and close the PR. 😢

@IgorMinar
Copy link
Contributor

thanks for the update.

@IgorMinar IgorMinar closed this Aug 28, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
browser: Firefox cla: yes component: $location needs: more info resolution: can't reproduce type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mackenco @caitp @IgorMinar @mary-poppins