Closed Bug 879133 Opened 7 years ago Closed 4 years ago

indexedDB.open fails if profile is in network drive

Categories

(Core :: Storage: IndexedDB, defect)

20 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox21 --- affected
firefox22 - affected
firefox23 - affected
firefox24 + affected
firefox49 --- fixed
firefox-esr17 --- unaffected

People

(Reporter: alice0775, Assigned: janv)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
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
Blocks: 820681
Keywords: regression
Version: Trunk → 20 Branch
I will investigate. However, right now, fail to see why Bug 820681 could impact IndexedDB in any way.
Also, can anyone check whether this is fixed in Firefox 22?
(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
(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
Blocks: 787804
No longer blocks: 820681
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.
Jan, do you want to take this?
Flags: needinfo?(Jan.Varga)
Yeah
Flags: needinfo?(Jan.Varga)
QA Contact: Jan.Varga
Assignee: nobody → Jan.Varga
QA Contact: Jan.Varga
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.
needsinfo on :janv here , for a quick update on the next steps here  as Fx24 is Aurora now.
Flags: needinfo?(Jan.Varga)
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 ...
Flags: needinfo?(Jan.Varga)
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.
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?
(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)
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?
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?
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()
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?
(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
Do you agree with the URI translation rules shown here:  http://www.sqlite.org/src/artifact/a7efdff151?ln=2181-2190
(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.
(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
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 ?
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).
(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
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?
(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.
(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 ?
What do you propose that SQLite do with non-localhost authorities in URIs?
Can't you just translate file://host/dir/file into \\host\dir\file ?
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?
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
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.
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 ?
Flags: needinfo?(Jan.Varga)
Depends on: SQLite3.8.0.2
Flags: needinfo?(Jan.Varga)
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.
Bad news, the new build time flag doesn't work well, I'll provide more details once I finish debugging
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.
(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.
IndexedDB also fails to open if the document is stored on a network path with the 5 slash pattern above. Same issue?
Any development in this? I am facing the same issue in FF 32 and 35
Hm, looks like this slipped off our radar somehow. Jan, what were the results of your debugging (comment 37)?
Flags: needinfo?(Jan.Varga)
I have to look at current sqlite and test it again ...
Flags: needinfo?(Jan.Varga)
Attached patch patch (obsolete) — Splinter Review
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 ?
Attachment #8571936 - Flags: feedback?(drh)
(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?
(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
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)
Attachment #8571936 - Flags: feedback?(drh) → feedback+
(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.
Attached patch patch (obsolete) — Splinter Review
Ok, m-c now uses a new version of sqlite with the allow uri authority changes and this patch actually enables it.
Attachment #8571936 - Attachment is obsolete: true
Attachment #8621494 - Flags: review?(bent.mozilla)
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.
Attachment #8621494 - Flags: review?(bent.mozilla) → review-
Duplicate of this bug: 1241420
Attached patch patch (obsolete) — Splinter Review
Attachment #8621494 - Attachment is obsolete: true
Flags: needinfo?(jvarga)
Attachment #8746487 - Flags: review?(khuey)
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.
Attachment #8746487 - Flags: review?(khuey)
Attachment #8746487 - Flags: review?(bugmail)
Attachment #8746487 - Flags: review+
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.
Attachment #8746487 - Flags: review?(bugmail) → review?(mak77)
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".
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 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.
Attachment #8746487 - Flags: review?(mak77) → review-
Attached patch patchSplinter Review
Attachment #8746487 - Attachment is obsolete: true
Attachment #8748103 - Flags: review?(mak77)
Attachment #8748103 - Flags: review?(mak77) → review+
Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d84b40b07cb7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.