Closed
Bug 1062005
Opened 10 years ago
Closed 10 years ago
provide thread-safe basic url parsing
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: bkelly, Assigned: bkelly)
Details
Attachments
(1 file)
753 bytes,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
As I understand it, we currently provide URI/URL parsing via nsIURI and nsIURL which can only be used from the main thread. I've been told this is largely due to the fact that nsIURI can be js implemented (bug 888604). Also, the IO service uses plugins to provide different protocol handlers which cannot be accessed off main thread.
My main question here is, is there any reason we could not implement this basic url parsing algorithm in a thread-safe way?
http://url.spec.whatwg.org/#concept-basic-url-parser
As far as I can tell, its protocol agnostic.
The reason I want this is that the ServiceWorker Cache object has the concept of "ignore the URL search field" on many operations. With the current nsIURI implementation this means I have to bounce to the main thread for nearly every operation or make the lookup extremely inefficient in the ignoreSearch case.
I'm willing to implement this algorithm, but I'd like to hear if people have objections or other ideas. Thanks.
Putting this in Netwerk since thats where NS_NewURI() and friends live.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Comment 1•10 years ago
|
||
> As far as I can tell, its protocol agnostic.
It's not. It has all sorts of places where it's scheme-dependent. See the scheme checks in "relative slash state", the "relative scheme" stuff in the scheme state and the things that depend on the "relative flag", probably others.
Now what we _could_ do is implement this spec in all its non-extensible glory. Assuming that's actually backwards-compatible...
Note that Jonas has a plan for making nsIURI available on background threads, so you may want to talk to him about timeframes.
Assignee | ||
Comment 2•10 years ago
|
||
Jonas, can you point me towards your plans for fixing off-main-thread URL manipulation? Thanks!
Flags: needinfo?(jonas)
Assignee | ||
Comment 3•10 years ago
|
||
I'm inclined to implement the Cache `ignoreSearch` feature only for http/https schemes initially. If/when we fix our off-main-thread URI story we can expand it to include other schemes.
I spoke with Jake on IRC and he seemed amenable to this plan. He had me open this issue:
https://github.com/slightlyoff/ServiceWorker/issues/440
Assignee | ||
Updated•10 years ago
|
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Comment 4•10 years ago
|
||
After looking at the code, I think that for the SW ignoreSearch feature for HTTP(S) URLs, all we need is to use nsStdURLParser. Its implementation doesn't do anything not thread safe that I can see as long as we don't pass references to it across threads (its AddRef/Release are not thread safe, but even that is easy to fix if we need to.)
The only complication is that this is an XPCOM service so we can't just do_CreateInstance() it on the background thread. However, I suggest exporting nsURLParsers.h and creating the C++ object directly.
Comment 5•10 years ago
|
||
Note that this stuff obviously won't work for add-on provided URL scheme parsers, but I don't think we need to block on that for SW (I am assuming that we don't support add-ons which provide their own way of parsing HTTP(S) URLs!)
Assignee | ||
Comment 6•10 years ago
|
||
I've verified that Ehsan's suggestion works for my particular use case.
This patch exports the nsURLParsers.h header from necko.
Patrick, for context, I want to use nsStdURLParser to extract the query from a URL off the main thread. At this point I know the URL is http/https and so does not need extended scheme support. I would like to avoid having to bounce to the main thread just to do this parse with NS_NewURI().
Attachment #8484455 -
Flags: review?(mcmanus)
Comment 7•10 years ago
|
||
Comment on attachment 8484455 [details] [diff] [review]
Export nsURLParsers.h to allow use of nsStdURLParser.
Review of attachment 8484455 [details] [diff] [review]:
-----------------------------------------------------------------
I really want thread safe uri functions too - thanks for working on this - it would be great to see a full interface available. The socket thread does some things with uris in strings for a couple of corner cases that it should really do with real nsIURIs for this same reason.
Attachment #8484455 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 8•10 years ago
|
||
The short of my plan is:
1. Add a way for addons to register a URL parser for a particular scheme. However rather than by
registering a callback which converts a string to an nsIURI instance, the API registers a set of flags
which indicate how the URI should be parsed. I.e. should it be treated like a URL with '/' separated
folders, or as a non-heirarchical URI (like data:), or like an origin with some data, (like blob:).
Or any other parsing methods that we think might make sense.
2. Deprecate nsIProtocolHandler.newURI and evangelize to addons to stop using it.
3. Deprecate all mutators on nsIURI/nsIURL, like the nsIURL filePath setter. If needed add functions on
nsIURI/nsIURL like "give me a new nsIURL but with filePath set to X".
4. Remove nsIProtocolHandler.newURI and nsIURI/nsIURL mutators after a release or two.
This way only our own parsers will be generating nsIURI instances. And those instances will be readonly. That should make it easy to make the nsIURI instances threadsafe, as well as make the uri parser threadsafe.
However obviously this is not a short-term plan.
Flags: needinfo?(jonas)
Comment 10•10 years ago
|
||
Assignee: nobody → bkelly
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•