New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Localstorage-down (alt) fails most attachment tests in Firefox #2098

Closed
adamshih opened this Issue Apr 29, 2014 · 20 comments

Comments

Projects
None yet
7 participants
@adamshih
Copy link
Contributor

adamshih commented Apr 29, 2014

But passing in Chrome?

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Apr 29, 2014

So, what you may be running into is some weirdnesses with escape()/unescape(). IIRC, Chrome was the only browser in which, for websql, I even had to do escape() at all. Safari didn't need it, and I don't know about Firefox because it doesn't have websql of course.

My hunch is that this will need to be solved in localstorage-down itself rather than on our end. Maybe call JSON.stringify() on everything before putting it in localStorage?

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Apr 29, 2014

OK, so it was for the PNG test. In Chrome, the string comes back as

"�PNG\r\n\u001a\n\u0000\u0000\u0000\rIHDR\u0000\u0000\u0000\u0010\u0000\u0000\u0000\u0010\b\u0003\u0000\u0000\u0000(-\u000fS\u0000\u0000\u00000PLTEþ÷èþôà þî�ýæºý�¤ü��û�bú¾Iú³,ù©\u0014ø¢\u0004ø�\u0000ø�\u0000ø�\u0000ø�\u0000ø�\u0000��tÿ\u0000\u0000\u0000pIDAT\u0018�M�Q\u0002�@\u0004C�Põáþ��0�móç�\u0004ê#�*¼c�#? \u0000��\u001f�æ\u000b­�G$ì:���`=\u0019®ð\ty@öm�èn\t�\\\u0010��?blkR\u0003h2�±\u0007\u0004�\u001fõ§\u0003<¹rw�\u0016ï�+iy��î<b�%��ô\u0003¢\u0005\b��L�¦\u0000\u0000\u0000\u0000IEND®B`�"

After decodeURIComponent(window.escape(str)), this is corrected and becomes:

"�PNG\r\n\u001a\n\u0000\u0000\u0000\rIHDR\u0000\u0000\u0000\u0010\u0000\u0000\u0000\u0010\b\u0003\u0000\u0000\u0000(-\u000fS\u0000\u0000\u00000PLTEþ÷èþôàþîÒýæºýÞ¤üÑ�ûÇbú¾Iú³,ù©\u0014ø¢\u0004ø�\u0000ø�\u0000ø�\u0000ø�\u0000ø�\u0000ÓÚtÿ\u0000\u0000\u0000pIDAT\u0018ÓM�Q\u0002Ä@\u0004CÃPõáþ×Ý0�móç�\u0004ê#�*¼c�#? \u0000�Å\u001fÜæ\u000b­×G$ì:À��`=\u0019®ð\ty@öm�èn\t\\\u0010��?blkR\u0003h2�±\u0007\u0004\u001fõ§\u0003<¹rwË\u0016ï�+iy�Ëî<bÆ%Ý�ô\u0003¢\u0005\b��LΦ\u0000\u0000\u0000\u0000IEND®B`�"

In WebSQL I solved this problem by putting a known value into the database and then checking what I got back. Based on that, I knew what encoding the database was using (UTF-16 in Safari, UTF-8 in Chrome), and whether or not it needed to be decoded when it came back. Maybe you'll have to do the same thing for Firefox/Chrome?

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Apr 29, 2014

In Chrome, when you pull attach out of the database, it's:

"�PNG\r\n\u001a\n\u0000\u0000\u0000\rIHDR\u0000\u0000\u0000\u0010\u0000\u0000\u0000\u0010\b\u0003\u0000\u0000\u0000(-\u000fS\u0000\u0000\u00000PLTEþ÷èþôà þî�ýæºý�¤ü��û�bú¾Iú³,ù©\u0014ø¢\u0004ø�\u0000ø�\u0000ø�\u0000ø�\u0000ø�\u0000��tÿ\u0000\u0000\u0000pIDAT\u0018�M�Q\u0002�@\u0004C�Põáþ��0�móç�\u0004ê#�*¼c�#? \u0000��\u001f�æ\u000b­�G$ì:���`=\u0019®ð\ty@öm�èn\t�\\\u0010��?blkR\u0003h2�±\u0007\u0004�\u001fõ§\u0003<¹rw�\u0016ï�+iy��î<b�%��ô\u0003¢\u0005\b��L�¦\u0000\u0000\u0000\u0000IEND®B`�" 

In Firefox it's:

""{\"storetype\":\"json\",\"data\":{\"type\":\"Buffer\",\"data\":[137,80,78,71,13,10,26,10,0,0,0,13,73,72,68,82,0,0,0,16,0,0,0,16,8,3,0,0,0,40,45,15,83,0,0,0,48,80,76,84,69,254,247,232,254,244,224,254,238,210,253,230,186,253,222,164,252,209,128,251,199,98,250,190,73,250,179,44,249,169,20,248,162,4,248,159,0,248,158,0,248,157,0,248,157,0,248,157,0,211,218,116,255,0,0,0,112,73,68,65,84,24,211,77,142,81,2,196,64,4,67,195,80,245,225,254,215,221,48,157,109,243,231,133,4,234,35,149,42,188,99,150,35,63,32,0,133,197,31,220,230,11,173,215,71,36,236,58,192,150,147,96,61,25,174,240,9,121,64,246,109,146,232,110,9,149,92,16,146,156,63,98,108,107,82,3,104,50,140,177,7,4,140,31,245,167,3,60,185,114,119,203,22,239,157,43,105,121,128,203,238,60,98,198,37,221,127,244,3,162,5,8,132,130,76,206,166,0,0,0,0,73,69,78,68,174,66,96,130]}}""

In IE10 it's:

"�PNG\r\n\u001a\n\u0000\u0000\u0000\rIHDR\u0000\u0000\u0000\u0010\u0000\u0000\u0000\u0010\b\u0003\u0000\u0000\u0000(-\u000fS\u0000\u0000\u00000PLTEþ÷èþôàþî�ýæºý�¤ü��û�bú¾Iú³,ù©\u0014ø¢\u0004ø�\u0000ø�\u0000ø�\u0000ø�\u0000ø�\u0000��tÿ\u0000\u0000\u0000pIDAT\u0018�M�Q\u0002�@\u0004C�Põáþ��0�móç�\u0004ê#�*¼c�#? \u0000��\u001f�æ\u000b­�G$ì:���`=\u0019®ð\ty@öm�èn\t�\\\u0010��?blkR\u0003h2�±\u0007\u0004�\u001fõ§\u0003<¹rw�\u0016ï�+iy��î<b�%��ô\u0003¢\u0005\b��L�¦\u0000\u0000\u0000\u0000IEND®B`�" 

In Safari you have to change this line or else Safari will just use WebSQL. It's:

"�PNG\r\n\u001a\n\u0000\u0000\u0000\rIHDR\u0000\u0000\u0000\u0010\u0000\u0000\u0000\u0010\b\u0003\u0000\u0000\u0000(-\u000fS\u0000\u0000\u00000PLTEþ÷èþôàþî�ýæºý�¤ü��û�bú¾Iú³,ù©\u0014ø¢\u0004ø�\u0000ø�\u0000ø�\u0000ø�\u0000ø�\u0000��tÿ\u0000\u0000\u0000pIDAT\u0018�M�Q\u0002�@\u0004C�Põáþ��0�móç�\u0004ê#�*¼c�#? \u0000��\u001f�æ\u000b­�G$ì:���`=\u0019®ð\ty@öm�èn\t�\\\u0010��?blkR\u0003h2�±\u0007\u0004�\u001fõ§\u0003<¹rw�\u0016ï�+iy��î<b�%��ô\u0003¢\u0005\b��L�¦\u0000\u0000\u0000\u0000IEND®B`�"

So it looks like Firefox is indeed the odd man out. I'm voting that we fix this in localstorage-down rather than in PouchDB (should be a simple decodeURIComponent(escape()) for non-FF), but I'm open to suggestions.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Apr 29, 2014

@calvinmetcalf @rvagg @maxogden In the browser, what is the LevelDOWN API supposed to guarantee in terms of the binary-type object we get out of the database? Is there a standard suite of tests we can run over level-js/memdown/localstorage-down to ensure that they're all exhibiting the same behavior?

I'm asking because what I want to avoid here is that we put a bunch of workarounds into PouchDB, when those things could be better enforced at the LevelDOWN level.

@calvinmetcalf

This comment has been minimized.

Copy link
Member

calvinmetcalf commented Apr 29, 2014

yes it's the abstract leveldown suite, though if I remember correctly there isn't much in the way of binary guarantees due to it being designed with indexedDB in mind which can handle all objects.

@daleharvey

This comment has been minimized.

Copy link
Member

daleharvey commented Apr 29, 2014

Thats one of the problems, chrome cant handle blobs yet, the patches are still landing (http://code.google.com/p/chromium/issues/detail?id=108012#c124)

And it seems leveldown should should try to enforce consistency and the browser differences dealt with in leveljs / localdown etc

@maxogden

This comment has been minimized.

Copy link
Contributor

maxogden commented Apr 29, 2014

abstract-leveldown only cares about Uint8Array and String (Buffer and String in node)

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented Apr 29, 2014

OK, so then anywhere on Node where leveldown would spit out or take in a Buffer, it should be a Uint8Array instead? That seems reasonable; looks like the browser support is pretty good.

@adamshih

This comment has been minimized.

Copy link
Contributor

adamshih commented May 1, 2014

Closing since #2097 fixed failing tests.

@adamshih adamshih closed this May 1, 2014

@maxogden

This comment has been minimized.

Copy link
Contributor

maxogden commented May 1, 2014

@nolanlawson I know it definitely works with http://npmjs.org/buffer (which is a modified Uint8Array in modern browsers and an Object in older browsers), I am not sure if it works with 'raw' Uint8Arrays though

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented May 1, 2014

OK, we'll look into that. Older browsers are the most important for us; the LocalStorage adapter is basically just for IE 8/9.

@rvagg

This comment has been minimized.

Copy link

rvagg commented May 2, 2014

You should definitely pull in @fno9 here too, I'm sure he'd be very keen to make sure that localstorage-down works for this use-case, this is a great way to push harder on the abstractions we're building here.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented May 2, 2014

@No9 thoughts?

@No9

This comment has been minimized.

Copy link

No9 commented May 7, 2014

localstorage-down is OOSS and very happy to see some flow here.
@adamshih can sit in
@isaacs how does one handover a repo to another user on npm?

@rvagg

This comment has been minimized.

Copy link

rvagg commented May 7, 2014

@No9 npm help owner, answers are all there

@No9

This comment has been minimized.

Copy link

No9 commented May 7, 2014

TY @rvagg @adamshih would you like to step in?

@adamshih

This comment has been minimized.

Copy link
Contributor

adamshih commented May 7, 2014

@No9 certainly not an expert on the subject matter, but I'm willing to try if you think that's appropriate.

Considering the long run, it might be better for someone else deeply involved with Pouch to step in instead of me, and/or maybe at least plan to do so in the future (I'm going to have a bunch going on in my personal/work life in the coming month + beyond and not sure what my free time will look like)? @nolanlawson might be interested in getting involved given all the levelalt stuff happening here?

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented May 7, 2014

I think pretty much anyone on the PouchDB team would be qualified to take this over. We're committed to using localstorage-down as a backend, so we'd have a vested interest in fixing any bugs/performance problems. If you'd like, @adamshih, we can be co-maintainers.

@No9: we're at https://www.npmjs.org/~nolanlawson and https://www.npmjs.org/~adamshih

@No9

This comment has been minimized.

Copy link

No9 commented May 7, 2014

Cool thanks for all your help on this folks.
So @nolanlawson I have added you as a contributor to the repo and I have added both users to npm owners so you can publish away.

@nolanlawson

This comment has been minimized.

Copy link
Member

nolanlawson commented May 7, 2014

@No9 OK thanks, we'll take good care of it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment