890089 - Bug 889442 causes timeouts in tests/test-content-script.test localStorage

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Test broke when bug 889442 landed.

TEST-UNEXPECTED-FAIL | tests/test-content-script.test localStorage | Test output exceeded timeout (60s).
Traceback (most recent call last):
  File "jetpack/bin/cfx", line 33, in <module>
    cuddlefish.run()
  File "C:\slave\test\build\jetpack\python-lib\cuddlefish\__init__.py", line 608, in run
    test_all_packages(env_root, defaults=options.__dict__)
  File "C:\slave\test\build\jetpack\python-lib\cuddlefish\__init__.py", line 490, in test_all_packages
    env_root=env_root)
  File "C:\slave\test\build\jetpack\python-lib\cuddlefish\__init__.py", line 937, in run
    pkgdir=options.pkgdir)
  File "C:\slave\test\build\jetpack\python-lib\cuddlefish\runner.py", line 745, in run_app
    OUTPUT_TIMEOUT, test_name, parseable)

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

[16:00]	<Mossop>	So data: uris no longer have localStorage?
	<KWierso|Home>	the bug only mentions about: pages
	I guess it applies to everything, though?
[16:03]	<Mossop>	Might be a bug with our tests testing against a chrome docshell as opposed to a content docshell

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Alex wrote that test, maybe he knows?

Comment 3

[Marco Bonardo [:mak]]

I only removed support for "about" and "moz-safe-about" schemes, what is the test doing?

Comment 4

[Dave Townsend [:mossop]]

(In reply to Marco Bonardo [:mak] from comment #3)
> I only removed support for "about" and "moz-safe-about" schemes, what is the
> test doing?

It's trying to use localStorage from a data: uri

Comment 5

[Marco Bonardo [:mak]]

I guess the test does not browse to a different page than about:home before running. So somehow it ends up trying to use localStorage of that page, and so far it was working thanks to this special supporting code. It should probably first navigate to another page that can use localStorage.

Comment 6

[Marco Bonardo [:mak]]

or, if not about:home, it's on another about page when it runs, maybe even blank? (not sure how that would behave though)

Comment 7

[Marco Bonardo [:mak]]

the other possibility is that this code was hiding a bug in the new DOM Storage implementation, I'm not sure regarding the DOM Storage spec for data uris, Honza may know better.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Commented in 889442 with this, but it looks like maybe one of the socialapi tests broke similarly? https://tbpl.mozilla.org/php/getParsedLog.php?id=24911878&tree=Mozilla-Inbound#error0

Comment 9

[Alexandre Poirot [:ochameau]]

I think that we are executing the test when being on the data: document.
As if I remember correctly, HiddeFrame.onReady is called after the about:blank document is ready.
I may be wrong but we can easily check this by ensuring here:
  https://github.com/mozilla/addon-sdk/blob/master/test/test-content-script.js#L23
that the document is the data: one.

Comment 10

[Marco Bonardo [:mak]]

how can this be an intermittent failure if it started failing consistently when a patch was pushed and stopped failing on backout?

Comment 11

[Ed Morley [:emorley]]

(In reply to Marco Bonardo [:mak] from comment #10)
> how can this be an intermittent failure if it started failing consistently
> when a patch was pushed and stopped failing on backout?

This bug matched my "someone forgot the intermittent failure keyword, so TBPL won't suggest the bug" bugzilla whine - and my skim read missed the comment 0 mention that this was caused by something that has since been backed out. (Bugs normally aren't filed for backouts & the backout changeset for some reason isn't mentioned in this bug). Duping to the bug which caused this, given now backed out.

Comment 12

[Marco Bonardo [:mak]]

no, this should stay open and block bug 889442

Comment 13

[Ed Morley [:emorley]]

Modifying summary to make this clear (both for me and the bugzilla whine), that this isn't a failure needing the keyword.

Comment 14

[Marco Bonardo [:mak]]

Mossop, is someone being assigned to work on this? I'm not totally sure I understand why this code is trying to use DOM storage from a data uri, but it's unlikely to work, it would end up using the about:blank principal that should not support that (it does just cause we special cased it) and it would not even support indexedDB.

Comment 15

[Dave Townsend [:mossop]]

Jordan is going to take a look here, thanks for the ping

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

This change also breaks other tests using localStorage and originating from non nsStandardURL (page-mod, page-worker), should have fixes by EOD

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: GH PR 1096

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #17)
> Created attachment 774299 [details]
> GH PR 1096

Is it just `tests/test-content-script.test localStorage` that is failing? this pull seems to be changing far more than just that.

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

No -- page-mod and page-worker also fail as they use localStorage in their tests on about:* or data:* pages, so those needed to be changed as well

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Marco Bonardo [:mak] from comment #14)
> Mossop, is someone being assigned to work on this? I'm not totally sure I
> understand why this code is trying to use DOM storage from a data uri, but
> it's unlikely to work, it would end up using the about:blank principal that
> should not support that (it does just cause we special cased it) and it
> would not even support indexedDB.

Hey Marco, I'm confused, is localStorage supported on data: uris or not?

I don't think the uri that was open before the data: uri should make a difference, and if it does than that seems like a platform bug.

Jordan I think we are try to test if a content script can access localStorage, if so then we can avoid all of these data: issues by using a httpd, if are really try to test that localStorage works with a data: uri then I don't think we should make the change in the review, we should test both the case that exists now and the tests that you have, which means we haven't solved this bug.

Comment 21

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #20)
> Jordan I think we are try to test if a content script can access
> localStorage, if so then we can avoid all of these data: issues by using a
> httpd, 

Or we can just test on resource: uris

Comment 22

[Marco Bonardo [:mak]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #20)
> Hey Marco, I'm confused, is localStorage supported on data: uris or not?
> 
> I don't think the uri that was open before the data: uri should make a
> difference, and if it does than that seems like a platform bug.

You may confirm this with a content hacker, I don't know the internal enough toi give a 100% informed answer, though I think data: uris have an about:blank principal, that is not suitable cause it lacks a host (it's a nsSimpleURI).
MDN just says "Prior to Gecko 6.0 (Firefox 6.0 / Thunderbird 6.0 / SeaMonkey 2.3), data URIs inherited the security context of the page currently in the browser window if the user enters a data URI into the location bar. Now data URIs get a new, empty, security context."
Som the behavior changed, in both cases though it wouldn't work 100% of the times so we'd still have the same issue, but thanks for pointing out the fact.

Comment 23

[Jordan Santell [:jsantell] [@jsantell]]

Correct, data: URIs do not have localStorage access after this platform change -- the reason why the tests were updated to use a resource:// uri instead of about:* and data:*. The reason why they were working before is because they were inheriting the principal of about:*, and they do not have a host to store the localStorage. I was under the impression that this is intentional (because of the nsISimpleURI)

Comment 24

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #23)
> Correct, data: URIs do not have localStorage access after this platform
> change -- the reason why the tests were updated to use a resource:// uri
> instead of about:* and data:*. The reason why they were working before is
> because they were inheriting the principal of about:*, and they do not have
> a host to store the localStorage. I was under the impression that this is
> intentional (because of the nsISimpleURI)

Isn't this test https://github.com/jsantell/addon-sdk/blob/7967455e19ef9b061409ea6bbf15168bdcee5c8d/test/test-content-script.js#L402-L425 assuming that data uris do have localStorage? 

I may not understand what's being said here, I certainly feel that way, but what I hear is that data: uris should not have localStorage, yet that is what is being tested in our tests, and the patch merely loads a resource: uri in the window before loading the a data: uri, instead of simply loading a data: uri which it does now.  It seems to me that our tests should just use a resource uri, when it comes to localStorage tests anyhow.

Mossop do you know someone who can give us a definitive answer?  bz maybe?

Comment 25

[Jordan Santell [:jsantell] [@jsantell]]

The `createProxyTest` helper creates an iFrame with a resource:// principal:
https://github.com/jsantell/addon-sdk/blob/7967455e19ef9b061409ea6bbf15168bdcee5c8d/test/test-content-script.js#L28

Then loads the data:uri once the frame's loaded for each test:
https://github.com/jsantell/addon-sdk/blob/7967455e19ef9b061409ea6bbf15168bdcee5c8d/test/test-content-script.js#L36

Comment 26

[Dave Townsend [:mossop]]

bz, should data: uris support localStorage?

Comment 27

[Boris Zbarsky [:bzbarsky]]

The key is not data: but who loaded the data:, no?  data: URIs tend to inherit their principal from whoever loaded them...

Comment 28

[Marco Bonardo [:mak]]

(In reply to Boris Zbarsky (:bz) from comment #27)
> The key is not data: but who loaded the data:, no?  data: URIs tend to
> inherit their principal from whoever loaded them...

Maybe it's just me but the security note at https://developer.mozilla.org/en-US/docs/data_URIs is not that clear, it sounds like they get no more an inherited context, while looks like they just get a somehow "clean" inherited context?

Comment 29

[Boris Zbarsky [:bzbarsky]]

That note only applies to URIs a user types in the location bar.

Comment 30

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Boris Zbarsky (:bz) from comment #29)
> That note only applies to URIs a user types in the location bar.

It's interesting that there is a distinction between the user typing a data: uri in to the location bar and a web page redirecting the window.location = 'data:'  ..

This is intentional for some reason tho?

Comment 31

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #30)
> (In reply to Boris Zbarsky (:bz) from comment #29)
> > That note only applies to URIs a user types in the location bar.
> 
> It's interesting that there is a distinction between the user typing a data:
> uri in to the location bar and a web page redirecting the window.location =
> 'data:'  ..
> 
> This is intentional for some reason tho?

The effect is the same afaict.

Comment 32

[Boris Zbarsky [:bzbarsky]]

The effect is different in terms of phishing.  This is the same reason that window.location = "javascript:..." works but typing "javascript:..." in the url bar mostly doesn't.

Comment 33

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Boris Zbarsky (:bz) from comment #32)
> The effect is different in terms of phishing.  This is the same reason that
> window.location = "javascript:..." works but typing "javascript:..." in the
> url bar mostly doesn't.

Ah alrighty, thanks for the clarification once again bz! :)

Comment 34

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/7967455e19ef9b061409ea6bbf15168bdcee5c8d
Bug 890089 Fix localStorage tests running on data:* and about:* URIs

https://github.com/mozilla/addon-sdk/commit/9ab379afd36f54f9c55e4d8bed22ca4f14dc0930
Merge pull request #1096 from jsantell/890089-fix-local-storage-tests

Bug 890089 Fix localStorage tests running on data:* and about:* URIs, r=@erikvold

Comment 35

[Marco Bonardo [:mak]]

when do the merges to central happen nowadays?

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Marco Bonardo [:mak] from comment #35)
> when do the merges to central happen nowadays?

I typically do them Thursdays or as needed.

Comment 37

[Marco Bonardo [:mak]]

Could you please notify me when this hits central, so I can proceed with the blocked bug? Thanks.

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Marco Bonardo [:mak] from comment #37)
> Could you please notify me when this hits central, so I can proceed with the
> blocked bug? Thanks.

Sorry for the delay. It should be in central today (and this time not get backed out).