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

RESOLVED FIXED

Status

Add-on SDK
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: KWierso, Assigned: jsantell)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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)
(Reporter)

Comment 1

5 years ago
[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
(Reporter)

Comment 2

5 years ago
Alex wrote that test, maybe he knows?
Flags: needinfo?(poirot.alex)
I only removed support for "about" and "moz-safe-about" schemes, what is the test doing?
(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
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.
or, if not about:home, it's on another about page when it runs, maybe even blank? (not sure how that would behave though)
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.
(Reporter)

Comment 8

5 years ago
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
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.
Flags: needinfo?(poirot.alex)

Updated

5 years ago
Keywords: intermittent-failure
how can this be an intermittent failure if it started failing consistently when a patch was pushed and stopped failing on backout?
Flags: needinfo?(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.
No longer blocks: 889442
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(emorley)
Keywords: intermittent-failure
Resolution: --- → DUPLICATE
Duplicate of bug: 889442
no, this should stay open and block bug 889442
Blocks: 889442
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Modifying summary to make this clear (both for me and the bugzilla whine), that this isn't a failure needing the keyword.
Summary: TEST-UNEXPECTED-FAIL | tests/test-content-script.test localStorage | Test output exceeded timeout (60s). → Bug 889442 causes timeouts in tests/test-content-script.test localStorage
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.
Flags: needinfo?(dtownsend+bugmail)
Jordan is going to take a look here, thanks for the ping
Assignee: nobody → jsantell
Flags: needinfo?(dtownsend+bugmail)
This change also breaks other tests using localStorage and originating from non nsStandardURL (page-mod, page-worker), should have fixes by EOD
Created attachment 774299 [details]
GH PR 1096
Attachment #774299 - Flags: review?(evold)
(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.
Flags: needinfo?(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
Flags: needinfo?(jsantell)
(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.
Flags: needinfo?(mak77)
(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
(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.
Flags: needinfo?(mak77)
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)
(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?
Flags: needinfo?(dtownsend+bugmail)
bz, should data: uris support localStorage?
Flags: needinfo?(dtownsend+bugmail) → needinfo?(bzbarsky)
The key is not data: but who loaded the data:, no?  data: URIs tend to inherit their principal from whoever loaded them...
Flags: needinfo?(bzbarsky)
(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?
That note only applies to URIs a user types in the location bar.
(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?
(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.
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.
(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! :)
Attachment #774299 - Flags: review?(evold) → review+

Comment 34

5 years ago
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
(Assignee)

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
when do the merges to central happen nowadays?
(Reporter)

Comment 36

5 years ago
(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.
Could you please notify me when this hits central, so I can proceed with the blocked bug? Thanks.
(Reporter)

Updated

5 years ago
Depends on: 897683
(Reporter)

Comment 38

5 years ago
(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).
Depends on: 903172
No longer depends on: 903172
You need to log in before you can comment on or make changes to this bug.