879133 - indexedDB.open fails if profile is in network drive

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/198e38876f7e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130603 Firefox/24.0 ID:20130603031140

This is maybe not regression because it does not work since Firefox16.

Steps To Reproduce:
1. Create Profile is network drive.
  So, profile.ini is like the following.
  [Profile1]
  Name=netprofile
  IsRelative=0
  Path=\\Machine\profile.netprofile
2. Start Firefox eith the profile
   Ex. Firefox.exe -P netprofile
3. Open URL or click "Test the online live demo" in https://developer.mozilla.org/ja/docs/IndexedDB/Using_IndexedDB
4. Type something in *fields and choose a image file
   And Click "Add Publication"

Actual Results:
 Nothing happen.
 Error in Error console as follow:

After Step3:
Error: UnknownError
Source File: https://mdn.mozillademos.org/en-US/docs/IndexedDB/Using_IndexedDB$samples/Full_IndexedDB_example?revision=422421
Line: 250

After Step4:
Error: TypeError: db is undefined
Source File: https://mdn.mozillademos.org/en-US/docs/IndexedDB/Using_IndexedDB$samples/Full_IndexedDB_example?revision=422421
Line: 278

Expected Results:
  Success.
  There are 1 record(s) in the object store.
  And Image appears left side pane.

Comment 2

[Alice0775 White]

In comment#0
> This is maybe not regression because it does not work since Firefox16.
is wrong.

Not work Firefox16.
Works Firefox17-19.
Not work Firefox20 and later.


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/c8a1314aa449
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121215 Firefox/20.0 ID:20121215131821
Bad:
http://hg.mozilla.org/mozilla-central/rev/ba26dc1c6267
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121217 Firefox/20.0 ID:20121217054022
Pushlog;
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8a1314aa449&tochange=ba26dc1c6267

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2992fbea1370
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121216 Firefox/20.0 ID:20121216162021
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1d268001c137
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121216 Firefox/20.0 ID:20121216164023
Pushlog;
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2992fbea1370&tochange=1d268001c137

Regressed by : Bug 820681

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I will investigate. However, right now, fail to see why Bug 820681 could impact IndexedDB in any way.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Also, can anyone check whether this is fixed in Firefox 22?

Comment 5

[Alice0775 White]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> Also, can anyone check whether this is fixed in Firefox 22?

What?

I confirmed that it is NOT working in Firefoc22b3 Aurora23.0a2 and Nightly24.0a1 :(
http://hg.mozilla.org/releases/mozilla-beta/rev/51c78e9573f4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 ID:20130528181031
http://hg.mozilla.org/releases/mozilla-aurora/rev/2229cf072c0d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130603 Firefox/23.0 ID:20130603004018
http://hg.mozilla.org/mozilla-central/rev/198e38876f7e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130603 Firefox/24.0 ID:20130603031140

Comment 6

[Alice0775 White]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> I will investigate. However, right now, fail to see why Bug 820681 could
> impact IndexedDB in any way.
OK, 

It fails even if Good builds, if corrupted database existed.
Then, it fails binary search to find the regression range.

So, I delete indexedDB folder every time to find the regression range..

Correct range is
Regression Window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/ba26dc1c6267
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121217 Firefox/20.0 ID:20121217054022
Bad:
http://hg.mozilla.org/mozilla-central/rev/2e70b718903a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121217 Firefox/20.0 ID:20121217162341
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ba26dc1c6267&tochange=2e70b718903a

Regression Window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/483fe8d512a0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121217 Firefox/20.0 ID:20121217085521
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2e70b718903a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121217 Firefox/20.0 ID:20121217120631
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=483fe8d512a0&tochange=2e70b718903a

Regressed by: Bug 787804

Comment 7

[Jan Varga [:janv]]

Yeah, it can be caused by bug 787804. We've switched to file URLs (from file paths) for opening SQLite databases. However, I don't know if it is a bug in our code or in SQLite. We'll need to investigated.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Jan, do you want to take this?

Comment 9

[Jan Varga [:janv]]

Yeah

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

We'll need to make sure this is addressed before the next ESR (24) but otherwise it's been in product since FF20 and has not raised a ton of user complaint so it seems like an edge case in general audience that doesn't require tracking until FF24.

Comment 11

[bhavana bajaj [:bajaj]]

needsinfo on :janv here , for a quick update on the next steps here  as Fx24 is Aurora now.

Comment 12

[Jan Varga [:janv]]

ok, here's the URI we pass to SQLite:

file://///hostabc/export/abcd1234.default/indexedDB/https+++mdn.mozillademos.org/idb/2895787499msdnno-idteamcoi-libn.sqlite?persistenceType=permanent&group=mozillademos.org&origin=https://mdn.mozillademos.org

obviously, those five slashes shouldn't be there
working on a fix ...

Comment 13

[Jan Varga [:janv]]

So I fixed the problem with extra slashes, but even when I pass correct URI to SQLite, it doesn't work.

From SQLite documentation:
"URI filenames are parsed according to RFC 3986. If the URI contains an authority, then it must be either an empty string or the string "localhost". If the authority is not an empty string or "localhost", an error is returned to the caller. The fragment component of a URI, if present, is ignored."

I'll ask SQLite devs about this restriction.

Comment 14

[D. Richard Hipp]

The restriction that the authority of a URI must be either omitted or "localhost" has been in SQLite since URI filenames were first supported with version 3.7.7 (circa 2011-06-24).  SQLite ignores the authority of the URI, so it would be harmless to remove this restriction.  However, a URI with a different authority (other than "localhost") seems incorrect.

We will be happy to add a compile-time option to SQLite to have it ignore this restriction, if it helps.  That compile-time option can go into SQLite version 3.8.0 which is due out in another month or so.  (FWIW, I'm entering this comment on FF nightly recompiled with SQLite 3.8.0 beta.)

But are you sure it is the malformed authority that is causing problems.  In the example shown in comment 12, the authority is an empty string, which SQLite should except.  In that example, the filename passed to the OS will be "///hostabc/export/abcd1234.default/indexedDB/https+++msdn.mozillademos.org/idb/2895787499msdnno-idteamcoi-libn.sqlite".  There are extra "/" characters at the beginning but they are ignore (at least for me on Linux - dunno about windows).

We've just discovered an anomaly in SQLite's handling of URIs:  The "+" characters is not translated into a space.  We are investigating now whether or not it ought to be.  As a work-around, please use "%20" instead of "+" for spaces in the URI filename.  Were you intending for the "+" characters in the URI of comment 12 to be spaces?

Comment 15

[Jan Varga [:janv]]

(In reply to D. Richard Hipp from comment #14)
> The restriction that the authority of a URI must be either omitted or
> "localhost" has been in SQLite since URI filenames were first supported with
> version 3.7.7 (circa 2011-06-24).  SQLite ignores the authority of the URI,
> so it would be harmless to remove this restriction.  However, a URI with a
> different authority (other than "localhost") seems incorrect.

well, I created following profile:
\\janvarga8bba\export\5i1bkpg1.default

"janvarga8bba" is the share name on windows
according to http://en.wikipedia.org/wiki/File_URI_scheme and http://blogs.msdn.com/b/ie/archive/2006/12/06/file-uris-in-windows.aspx the URI should be:
file://janvarga8bba/export/5i1bkpg1.default

we have a bug in file URL implementation, so we currently produce this url:
file://///janvarga8bba/export/5i1bkpg1.default

I fixed it in my local dev tree

> 
> We will be happy to add a compile-time option to SQLite to have it ignore
> this restriction, if it helps.  That compile-time option can go into SQLite
> version 3.8.0 which is due out in another month or so.  (FWIW, I'm entering
> this comment on FF nightly recompiled with SQLite 3.8.0 beta.)

that sounds good to me

> 
> But are you sure it is the malformed authority that is causing problems.  In
> the example shown in comment 12, the authority is an empty string, which
> SQLite should except.  In that example, the filename passed to the OS will
> be
> "///hostabc/export/abcd1234.default/indexedDB/https+++msdn.mozillademos.org/
> idb/2895787499msdnno-idteamcoi-libn.sqlite".  There are extra "/" characters
> at the beginning but they are ignore (at least for me on Linux - dunno about
> windows).

that url is incorrect, I fixed it and SQLite still returns an error
I think SQLite should internally translate the URI back to:
\\janvarga8bba\export\5i1bkpg1.default
on windows only indeed

> 
> We've just discovered an anomaly in SQLite's handling of URIs:  The "+"
> characters is not translated into a space.  We are investigating now whether
> or not it ought to be.  As a work-around, please use "%20" instead of "+"
> for spaces in the URI filename.  Were you intending for the "+" characters
> in the URI of comment 12 to be spaces?

those "+" characters are real "+" characters (not spaces)

Comment 16

[D. Richard Hipp]

What is the string that you are sending to SQLite now? What error is it returning?  What should the underlying filename be?  Are the two \\ characters at the beginning of the filename required, or would a single \ character be sufficient?  How do we know when two \\ characters are required and when only one \ can be used?

Comment 17

[D. Richard Hipp]

Does the comment at http://www.sqlite.org/src/artifact/ac440b6de?ln=2184-2187 correctly describe how URI filenames ought to be interpreted on windows and on unix?

Comment 18

[Jan Varga [:janv]]

I actually think that changing file URL building and parsing in our code could cause many regressions, so those "five slashes" seem to be ok in the end.
When I paste "file://///janvarga8bba/export" to Internet Explorer on Windows it translates the url into "\\janvarga8bba\export"

So maybe we could just teach SQLite to work with file://///

I debugged SQLite code a bit, here are some observations:

uri passed to openDatabase():
file:///C:/Documents%20and%20Settings/Administrator/Application%20Data/Mozilla/Firefox/Profiles/5i1bkpg1.default/indexedDB/https+++mdn.mozillademos.org/idb/2895787499msdnno-idteamcoi-libn.sqlite?persistenceType=permanent&group=mozillademos.org&origin=https://mdn.mozillademos.org

path passed to sqlite3BtreeOpen():
/C:/Documents and Settings/Administrator/Application Data/Mozilla/Fire
fox/Profiles/5i1bkpg1.default/indexedDB/https+++mdn.mozillademos.org/idb/2895787
499msdnno-idteamcoi-libn.sqlite

the "/" before "C:" gets removed in winFullPathname(), so the final file path is ok


Now when the profile is on a network drive ...

uri passed to openDatabase():
file://///janvarga8bba/export/5i1bkpg1.default/indexedDB/https+++mdn.mozil
lademos.org/idb/2895787499msdnno-idteamcoi-libn.sqlite?persistenceType=permanent
&group=mozillademos.org&origin=https://mdn.mozillademos.org

path passed to sqlite3BtreeOpen():
///janvarga8bba/export/5i1bkpg1.default/indexedDB/https+++mdn.mozillad
emos.org/idb/2895787499msdnno-idteamcoi-libn.sqlite

the extra "/" doesn't get removed in winFullPathname()

Comment 19

[D. Richard Hipp]

Is "file://///janvarga8bba/export" considered a "correct" URI that ought to be translated into "\\janvarga8bba\export", or is it just that IE happens to make that translation as part of its error recovery?

Comment 20

[Jan Varga [:janv]]

(In reply to D. Richard Hipp from comment #19)
> Is "file://///janvarga8bba/export" considered a "correct" URI that ought to
> be translated into "\\janvarga8bba\export", or is it just that IE happens to
> make that translation as part of its error recovery?

It's not 100% correct, but some applications use it according to "Legacy URLs" section in [1]. Firefox on windows accepts that URI too.

[1] http://en.wikipedia.org/wiki/File_URI_scheme

Comment 21

[D. Richard Hipp]

Do you agree with the URI translation rules shown here:  http://www.sqlite.org/src/artifact/a7efdff151?ln=2181-2190

Comment 22

[Jan Varga [:janv]]

(In reply to D. Richard Hipp from comment #21)
> Do you agree with the URI translation rules shown here: 
> http://www.sqlite.org/src/artifact/a7efdff151?ln=2181-2190

Yes! It looks good to me, but let me actually test the new SQLite code.

Comment 23

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #22)
> (In reply to D. Richard Hipp from comment #21)
> > Do you agree with the URI translation rules shown here: 
> > http://www.sqlite.org/src/artifact/a7efdff151?ln=2181-2190
> 
> Yes! It looks good to me, but let me actually test the new SQLite code.

it seems to work correctly
I verified that the IndexedDB demo works on windows when profile is on a network drive.

try looks good too:
https://tbpl.mozilla.org/?tree=Try&rev=57b561a34a00

Comment 24

[Jan Varga [:janv]]

So I guess we're not going to wait for new SQLite release ...
D. Richard Hipp: is http://www.sqlite.org/src/info/de05eb75ec the final patch ?

Comment 25

[D. Richard Hipp]

If there is no push-back from other users, then I'll merge that change onto the SQLite trunk and it will become part of the next release (version 3.8.0).

Comment 26

[Jan Varga [:janv]]

(In reply to D. Richard Hipp from comment #25)
> If there is no push-back from other users, then I'll merge that change onto
> the SQLite trunk and it will become part of the next release (version 3.8.0).

Ok, so let's wait for new SQLite version on the trunk and possibly patch ESR 24 once there's a branch for it.

We can also land the fix on trunk right now if release drivers think that this should be fixed ASAP

Comment 27

[D. Richard Hipp]

I'm getting pushback from the SQLite community.  People are telling me that if the URI is not well-formed then SQLite should throw an error, not try to cope as the patch above does.  And that argument does make sense.  So the patch might not make it into the SQLite core.

Are you sure you can't fix FF to generate a well-formed URI?

Comment 28

[Jan Varga [:janv]]

(In reply to D. Richard Hipp from comment #27)
> I'm getting pushback from the SQLite community.  People are telling me that
> if the URI is not well-formed then SQLite should throw an error, not try to
> cope as the patch above does.  And that argument does make sense.  So the
> patch might not make it into the SQLite core.
> 
> Are you sure you can't fix FF to generate a well-formed URI?

Well, I can add a hack on our side, something like:
if (StringBeginsWith(spec, NS_LITERAL_CSTRING("file://///"))) {
  spec.Cut(7, 3);
}

before:
int srv = ::sqlite3_open_v2(spec.get(), &mDBConn, mFlags, NULL);
if (srv != SQLITE_OK) {
  mDBConn = nullptr;
  return convertResultCode(srv);
}

But there's still the problem that SQLite doesn't accept non localhost authorities.
So that's the minimum (on SQLite side) we need to fix this bug.

Comment 29

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #28)
> But there's still the problem that SQLite doesn't accept non localhost
> authorities.
> So that's the minimum (on SQLite side) we need to fix this bug.

drh:
So, is that acceptable ?

Comment 30

[D. Richard Hipp]

What do you propose that SQLite do with non-localhost authorities in URIs?

Comment 31

[Jan Varga [:janv]]

Can't you just translate file://host/dir/file into \\host\dir\file ?

Comment 32

[D. Richard Hipp]

It is unclear if that would work or not.  I don't understand the Uniform Naming Convention myself, but people tell me that it is more complicated than that. Furthermore, SQLite should not operate differently on windows than it does on other platforms, and since support for UNC is mainly restricted to windows it seems ill-advised to support UNC on windows when it is not supported anywhere else.

I'm not saying that translating non-localhost authorities as you suggest is wrong, I'm only saying that I don't know that it is right.  And so many other projects depend on SQLite being backwards compatible at all times that we have to get it right, because once something like this goes in, it cannot be changed.  So, following the principle of "first do no harm", I do not want to make this change.

Now, if you have a need to support non-localhost authorities on windows, and if you can convince the broader SQLite community that adding support for it is a good idea, then I'm all for it.  But when I suggested making this change on the SQLite developers mailing list, I got a lot of negative feedback.  People who appear to know more about UNC than I do seem to believe that my prior attempt at doing something like that was misguided.  I don't fully understand the objections, but until I do understand them I'm going to leave things as they stand.

Can you better articulate why you need SQLite to support non-localhost authorities?  What problems would this solve?

Comment 33

[Jan Varga [:janv]]

We had to switch to file URIs, especially to be able to pass custom parameters to our own VFS implementation. Everything works well, except that some windows users use profiles stored on a network drive (e.g. \\host\profiles\myprofile). All IndexedDB data is stored in user's profile so the URI we pass to SQLite looks like this:

file://///host/profiles/myprofile/indexedDB/https+++mdn.mozillademos.org/idb/2895787499msdnno-idteamcoi-libn.sqlite?persistenceType=permanent&group=mozillademos.org&origin=https://mdn.mozillademos.org

I could add a hack so we wouldn't pass the "incorrect" URI above, but a correct one:
file://host/profiles/myprofile/indexedDB/https+++mdn.mozillademos.org/idb/2895787499msdnno-idteamcoi-libn.sqlite?persistenceType=permanent&group=mozillademos.org&origin=https://mdn.mozillademos.org

the problem is that "host" is not accepted, only an empty string or "localhost"

We have multiple options here:
1. Do nothing
- some users won't be happy

2. Revert our IndexedDB implementation to use file paths
- very painful, if possible at all
- using file URIs is very nice solution how to pass parameters to a VFS

3. Allow non-localhost authorities in SQLite
- as you mentioned, hard to convince the broader SQLite community

4. Allow non-localhost authorities in SQLite if a certain flag is set
- e.g. SQLITE_OPEN_ALLOW_NON_LOCALHOST_AUTHORITIES, or something shorter :)

I vote for 4.
This way, it shouldn't affect other projects depending on SQLite

Comment 34

[D. Richard Hipp]

OK.  The #define is SQLITE_ALLOW_URI_AUTHORITY.  This will be in the upcoming 3.8.0 release.
You can get a snapshot of the latest from http://www.sqlite.org/download.html.  Please let me know if for some reason this does not work for you.

Comment 35

[bhavana bajaj [:bajaj]]

needinfo on janv, for with with next steps given we tracked this for Firefox 24 which is the ESR based on https://bugzilla.mozilla.org/show_bug.cgi?id=879133#c10 ?

Comment 36

[Landry Breuil (:gaston)]

Does this also mean that a configure check will be added to ensure people building with systemwide sqlite (which is often the case on *nix OSes) have this option turned on, as it was done for SECURE_DELETE ? since it seems to only affect windows users...

Note that i'm not asking for such a check, which is painful to circumvent... but the case is different than SECURE_DELETE which has a performance penalty.

Comment 37

[Jan Varga [:janv]]

Bad news, the new build time flag doesn't work well, I'll provide more details once I finish debugging

Comment 38

[Reed Jackson]

hi, we use Mac OS X 10.8.5 and Network Homes, and Managed Client (Apple's Network Profiles) and we are having a problem with the Profiles being stored on the network homes. you can create this problem by taking an existing network homed user, upgrade to FF 24.  Then launch FF 24, and the app seizes up, it will draw a browser window, but no contents.

I found a work around. Launch the profile manager from the command line, and move the profile to a local location on the startup drive.  

I wonder if the fix you applied for the Windows folks, breaks the use of the Mac OS X users when used in an environment like ours... network homes.

So, for instance the profile.ini is  Relative=1 and path = /Profiles/dadsfadf.default

I changed it to Relative=0 and path = /Users/Shared/Firefox_profile_fix

This allows it to work.

Let me know if you want some richer info to assist with any further pinpointing.

Comment 39

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Reed Jackson from comment #38)
> hi, we use Mac OS X 10.8.5 and Network Homes, and Managed Client (Apple's
> Network Profiles) and we are having a problem with the Profiles being stored
> on the network homes. you can create this problem by taking an existing
> network homed user, upgrade to FF 24.  Then launch FF 24, and the app seizes
> up, it will draw a browser window, but no contents.


I think you rather want to follow bug 918612.

Comment 40

[XFox]

IndexedDB also fails to open if the document is stored on a network path with the 5 slash pattern above. Same issue?

Comment 41

[Avisek Jena]

Any development in this? I am facing the same issue in FF 32 and 35

Comment 42

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm, looks like this slipped off our radar somehow. Jan, what were the results of your debugging (comment 37)?

Comment 43

[Jan Varga [:janv]]

I have to look at current sqlite and test it again ...

Comment 44

[Jan Varga [:janv]]

Attachment: patch

The current sqlite contains only a part of the original patch which worked for me.
I'm attaching additional changes (ignore moz.build changes) that are needed to fix this bug.
drh, do you think it can be incorporated into official sqlite ?

Comment 45

[D. Richard Hipp]

(In reply to Jan Varga [:janv] from comment #44)
> I'm attaching additional changes (ignore moz.build changes) that are needed
> to fix this bug.
> drh, do you think it can be incorporated into official sqlite ?

We are certainly open to enhancing SQLite so that it is able to deal with dodgy URIs containing extra "/" characters, especially if that is a common error.  But I'm not sure the patch you provide is exactly right.  Your patch appears to omit the //localhost/ prefix regardless of whether SQLITE_ALLOW_URI_AUTHORITY is defined, but to do it in different ways depending on whether or not it is defined.  I don't really understand UNC filenames, so I don't know if this is right or wrong, but I think it requires careful investigation before moving forward.

Would it work better if you changed FF to disable SQLITE_ALLOW_URI_AUTHORITY and we enhance the code that discards the scheme and authority segments of the URI so that it correctly deals with the mal-formed URIs with five "/" characters?

We originally added the SQLITE_ALLOW_URI_AUHORITY compile-time option for FF.  But I do not recall what it accomplishes.  Can you refresh my memory?

Comment 46

[Jan Varga [:janv]]

(In reply to D. Richard Hipp from comment #45)
> (In reply to Jan Varga [:janv] from comment #44)
> > I'm attaching additional changes (ignore moz.build changes) that are needed
> > to fix this bug.
> > drh, do you think it can be incorporated into official sqlite ?
> 
> We are certainly open to enhancing SQLite so that it is able to deal with
> dodgy URIs containing extra "/" characters, especially if that is a common
> error.  But I'm not sure the patch you provide is exactly right.  Your patch
> appears to omit the //localhost/ prefix regardless of whether
> SQLITE_ALLOW_URI_AUTHORITY is defined, but to do it in different ways
> depending on whether or not it is defined.  I don't really understand UNC
> filenames, so I don't know if this is right or wrong, but I think it
> requires careful investigation before moving forward.
> 
> Would it work better if you changed FF to disable SQLITE_ALLOW_URI_AUTHORITY
> and we enhance the code that discards the scheme and authority segments of
> the URI so that it correctly deals with the mal-formed URIs with five "/"
> characters?
> 

This is exactly what we did originally, there's was no SQLITE_ALLOW_URI_AUTHORITY and your proposed patch for sqlite handled uri authorities and also five "/", but then you said that other sqlite developers don't like the patch, so we decided to come up with SQLITE_ALLOW_URI_AUTHORITY. However, the five "/" part of the fix somehow didn't make it into official sqlite.

> We originally added the SQLITE_ALLOW_URI_AUHORITY compile-time option for
> FF.  But I do not recall what it accomplishes.  Can you refresh my memory?

There are two problems at least, one is that sqlite currently doesn't allow anything but "localhost" as uri authority. So we can't use for example "\\mycomputer\myshare", only "\\localhost" is allowed. The second problem is, that sqlite doesn't allow urls with five "/", for example file://///mycomputer/myshare

Comment 47

[D. Richard Hipp]

OK.  Changed checked in here:  https://www.sqlite.org/src/info/39b566a2d0916c57

Do you need a special patch release of SQLite for this? (That would be SQLite 3.8.8.4.)  Or can you wait until the next official release, due sometime in April?

Is the documentation for SQLITE_ALLOW_URI_AUTHORITY still correct in your opinion?  (https://www.sqlite.org/compile.html#allow_uri_authority)

Comment 48

[Jan Varga [:janv]]

(In reply to D. Richard Hipp from comment #47)
> OK.  Changed checked in here: 
> https://www.sqlite.org/src/info/39b566a2d0916c57

Thanks!

> 
> Do you need a special patch release of SQLite for this? (That would be
> SQLite 3.8.8.4.)  Or can you wait until the next official release, due
> sometime in April?

I think we can wait till April.

> 
> Is the documentation for SQLITE_ALLOW_URI_AUTHORITY still correct in your
> opinion?  (https://www.sqlite.org/compile.html#allow_uri_authority)

I think it's still correct.

Comment 49

[Jan Varga [:janv]]

Attachment: patch

Ok, m-c now uses a new version of sqlite with the allow uri authority changes and this patch actually enables it.

Comment 50

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8621494 [details] [diff] [review]
patch

Review of attachment 8621494 [details] [diff] [review]:
-----------------------------------------------------------------

You'll need to modify the configure.in checks for system sqlite also.

Comment 52

[Jan Varga [:janv]]

Attachment: patch

Comment 53

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8746487 [details] [diff] [review]
patch

Review of attachment 8746487 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.  I'd like asuth to take a look at it too.

Comment 54

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8746487 [details] [diff] [review]
patch

=== Short

The configure.in stuff seems correctly copied/pasted/transformed and the define seems to have no gotchas.  The big question is whether it's appropriate to issue this define for all platforms and require system SQLite consumers who are probably not on Windows to build their SQLite with the define that is irrelevant to their platform.

This seems like a module owner call, so transferring the r? over to :mak.

=== Analysis Dump

This is my understanding of things:

- The correct way to URI-encode the windows UNC path \\host\path is file://host/path (per https://blogs.msdn.microsoft.com/ie/2006/12/06/file-uris-in-windows/) but in comment 18, Jan indicated (quite reasonable) concerns about regressions, so we still generate file://///host/path with this patch.

- SQLite added a SQLITE_ALLOW_URI_AUTHORITY define just for us.  Ostensibly per https://www.sqlite.org/compile.html#allow_uri_authority this just allows authority other than empty or "localhost", but looking at the source, things get slightly more specialized for us so it bears analysis that you can skip over because everything's fine.  (See https://github.com/mackyle/sqlite/blob/fbb55bcef3cd7a9a78d4f9b4c9e9e21f99b613dd/src/main.c#L2526 because the source viewer at sqlite.org just wanted to do a big <pre> block and annotate only generated a subset of anchors.)
  - With SQLITE_ALLOW_URI_AUTHORITY defined:
    - Special-cases five slashes so that "file://///host/path" is transformed into a filename of "//host/path".
    - Special-cases localhost so that "file://localhost/path" is transformed into a filename of "/path"
    - Special-cases three(+) slashes so that "file:///path" is transformed into "/path"
    - Falls-through so that illegal strings "file://path" becomes "//path", "file:/path" becomes "/path", and "file:path" becomes "path"
  - Without that define:
    - Special-cases 2+ slashes with authority-detecting logic.  Input without an authority such as "file:///path" becomes "/path".  Input with an authority explodes if the authority is not "localhost".  Illegal strings of the form "file://path" also explode.
    - That special-case handles 5 slashed input like "file://///host/path" by turning it into "///host/path".
    - Falls-through so that illegal string "file:/path" becomes "/path" and "file:path" becomes "path".
  - So the main difference in behavior is the handling of illegal string "file://path".  With the define, we get "//path", without the define things break.
    - Does this matter to Gecko somehow?  No.  nsLocalFileUnix requires all paths start with "/" and both nsURLHelperUnix.cpp and nsURLHelperOSX prepend "file://" so we get "file:///path".  And nsURLHelperWin.cpp prepends "file:///" in all cases.
    - Does this matter to linux distros?  This part, probably not?

- We're adding this define for all platforms even though it only seems relevant for Windows.  Even if we were building with some magic VFS abstraction like GVfs, I don't think the define would help those cases.  This is relevant because it requires those building Firefox to add this define to their system SQLite.  It's not clear Gecko derives any benefit from doing this across all platforms other than avoiding wrapping the configure check in a 'case "${OS_TARGET}" in\nWINNT)'.  It does seem to result in work for distros and encourages a debate as to whether they should issue this define.  If they decide to try and patch it out, that's still a hassle for them and could have negative side-effects down the road.

Comment 55

[Andrew Sutherland [:asuth] (he/him)]

Omitted not-quite-implied detail: With the define of SQLITE_ALLOW_URI_AUTHORITY, the correct "file://host/path" syntax also comes out like "//host/path".

Comment 56

[Jan Varga [:janv]]

IIRC, without SQLITE_ALLOW_URI_AUTHORITY, our IndexedDB doesn't work at all, but I agree that it can be defined just for windows.

Comment 57

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8746487 [details] [diff] [review]
patch

Review of attachment 8746487 [details] [diff] [review]:
-----------------------------------------------------------------

Andrew is right, this breaks system sqlite, and we can't expect everyone to add the define in a timely manner, since it doesn't seem to bring them any benefit.

These are the possible choices when we hit these roadblocks:

1. drop system sqlite support:
has been discussed over and over. I think sooner or later we'll reach a point where we won't be able to avoid that... but we're not there yet.

2. ask everyone to build with our options:
This is something we could do with things that could be useful to multiple projext, for example if we'd decide we want the ICU or REGEXP extensions built-in, it's clear other software may find those useful and maybe some distros are indeed already building with those. It was the case for the SAFE_DELETE define (even if some distro complained about the perf hit, it had a good privacy benefit). This is not the case, this fix is required for Windows, and we'd be asking linux distros to take it, just for us.

3. workaround the thing:
In this case, looks like we could wrap this into an ifdef just for Windows, and that doesn't look very expensive to do, it's likely less expensive than spending hours discussing with distro maintainers.

So, if that is enough to solve this problem, please make the define only for Windows and avoid the system sqlite check in configure.in. Otherwise, we'll re-evaluate 2.

Comment 58

[Jan Varga [:janv]]

Attachment: patch

Comment 59

[Jan Varga [:janv]]

Thanks!

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d84b40b07cb7

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d84b40b07cb7