Review of Web Request and Navigation API Proposal

[Mike Perry]

Hi,

I'm the author of the Torbutton Firefox addon, and a co-author of the
HTTPS Everywhere addon, two addons that make heavy use of Firefox's
numerous request and response observer APIs.

My brother Matt, as well as Jochen, Pam, and Bernhard asked that I
review the Web Request API for functionality and performance.

I have a few general comments about usability, and also some comments
on performance enhancements to make the blocking versions possible. I
am particularly interested in the blocking use case for these APIs,
which are used by a large number of extremely popular Firefox addons.
In fact, as far as I know, by pure user count, the overwhelming use of
the Firefox observer APIs is in a blocking fashion.

Popular examples include Adblock Plus, NoScript, FoxyProxy,
RequestPolicy, UserAgentSwitcher, RefSpoof, HTTPS Everywhere, Tamper
Data, and Torbutton, amongst others. The most popular of these addons
is AdBlock Plus, with approximately 10 million daily users, and the
least popular probably is Torbutton, with approximately 400k daily
users.

First, my comments about the general API format:

1. tabID should be available to all of these APIs

Currently, it appears as though addons are expected to subscribe to
onBeforeRequest in order to associate tabIDs to requestIDs. This can
introduce a performance penalty for addons that need tabIDs but do not
otherwise need to listen to onBeforeRequest. It can also introduce
instability if addons are not able to update their tabID to requestID
map before subsequent events are delivered.

2. tabID should never be NULL/0/unknown without other hints

Many addons actually do care about browser-generated requests such as
autocomplete, search bar requests, and other addon requests, and would
like to be able to recognize them as such.

For developer sanity and also for debugging, different parts of the
browser should be given special constant tabIDs, or there should be
special requestInfo request types provided whenever the tabID is null.

This will also help find odd edge cases deep in the code where the
tabID is omitted due to oversight or developer ignorance/laziness.
These cases are quite common in Firefox, and they are maddening to
deal with from an extension developer's perspective.

3. The requestID must remain constant across redirect loops

From the API spec, it sounds like this requestID will be constant even
across redirects, but its hard to tell for sure. I could easily see
certain redirect cases causing the implementation to forget to
preserve the requestID. This would be very bad.

In Firefox, they actually expose a redirect counter to you through
XPCOM, and you can see how many redirects a URL has left before the
browser gives up. This is pretty important information to have when
you are rewriting URLs arbitrarily.

4. onDOMContentLoaded may need multiple events (or maybe none)

One of the frustrating things about Firefox is being able to intercept
page load at points that make sense to an addon developer as opposed
to a browser implementer. In terms of the DOM, most addon developers
care about 3 points: the DOM is not yet built but you can now insert
your script and/or objects; the DOM is fully built but absolutely no
page scripts have run yet; and the DOM is built and all DOM loaded
events have been dispatched to page script. However, Firefox exposes
about a dozen different events here, none of which clearly map to this
simple model in terms of DOM events.

Maybe this is the wrong API set for any DOM-related events though, and
these types of events are only relevant to content scripts who
actually manipulate the DOM. Or maybe all three events should be
present in both places. Something to consider.

5. webNavigation.onBeforeNavigate should always fire before
webRequest.onBeforeRequest.

For many addons, it is very important to be able to tell if a request
was due to navigation, or due to some automatic load condition.

This is particularly problematic with async events though. If
onBeforeNavigate fires, but it is possible that an extension does so
much work there that it doesn't finish before onBeforeRequest is
delivered, this will cause unexpected behavior in addons that depend
on this knowledge.

If this cannot be avoided due to the event+process model, there should
be a flag added to the extraInfo object in onBeforeRequest to indicate
the request was generated due to a navigation. If this is already what
the "main_frame" request type is supposed to provide, then please
ignore this point entirely :)

6. Host "*" should be allowed in the permissions.

There will be addons that are interested in the filter * for their
host pattern for the permissions for these APIs, so that should
definitely be allowed, namely addons whose filter lists change
dynamically.

In fact, most of the addons mentioned above have this property, or
otherwise require universal access to these APIs.


That covers my general comments. Now, on to comments about the
blocking versions of the APIs.

Again, the set of addons that will need blocking capabilities is very
substantial, but not all APIs need blocking versions. In particular,
the addons I mentioned at the beginning of this mail would need the
following:

1. WebRequest.onBeforeRequest

In terms of modification, the above addons use a combination of
"cancel", header modification, and destination URL modification.

2. WebRequest.onHeadersReceived

Tamper Data (and possibly others) will want header modification at
this stage.

3. WebRequest.onBeforeRedirect

Almost all of the above addons will want "cancel", header
modification, and redirect destination URL modification.


Now the obvious problem here is that if we have multiple blocking
APIs, and potentially a dozen or so addons all subscribing them, page
load times will become noticeably slower.

The solution I propose is to simulate synchronous execution through a
conflict resolution step after sending out events to all extensions in
parallel, and have them respond with copies of the objects they wish
to change/cancel. If multiple extensions respond with modified objects
for the url or for the same HTTP header lines, break ties using an
internal priority number assigned to each extension based on its
position in a list sorted by extension name/GUID.

Because all extensions would run in parallel, this keeps the total
overhead of the blocking APIs constant, regardless of the number of
subscribers. Also, because only 2 of the 3 blocking APIs are regularly
called in normal use, and because each has a requestFilter lambda that
prevents them from being called unless the addon is actually
interested in that URL at runtime, the overhead should be reasonably
bounded.

The other key property is that the fixed ordinal resolution step
simulates the behavior a user would get from a synchronous Firefox-
style "observer list". This also means that in the case of an addon
conflict, the conflict will always happen deterministically with the
same behavior each time. Since the ordering is always the same with
respect to two addons, the typical user experience of disabling/
enabling a particular addon to diagnose problems will always produce
the same results, even on different computers. This is key for
debugging and diagnosis.

The same priority resolution mechanism can be used for other blocking
APIs, such as an equivalent to Firefox's nsIProtocolProxyFilter.

[Erik Kay]

Hi Mike.  Thanks for the detailed feedback.  I'll let others respond to the rest of your points, but I wanted to talk about one in particular.

I don't think it's a safe assumption that this API should allow modification of non-page-generated requests.  It exposes the browser to a range of attacks that it isn't open to today, so I think this is too dangerous.  It's possible that we could provide specific features to accomplish your use cases in a safer way though (for example, maybe we could allow blocking but not modification).  Could you give us an idea of what you'd like to do in these cases?

 

--
You received this message because you are subscribed to the Google Groups "Chromium-extensions" group.
To post to this group, send email to chromium-...@chromium.org.
To unsubscribe from this group, send email to chromium-extens...@chromium.org.
For more options, visit this group at http://groups.google.com/a/chromium.org/group/chromium-extensions/?hl=en.

[Yuzhu Shen]

[Note: reply-to-all will go to open-source community.]

Hi, Mike.

Thanks a lot for your review comments!

The following are my personal opinions:

On Mon, Aug 9, 2010 at 4:55 PM, Mike Perry <mikeperr...@gmail.com> wrote:

Personally I am fine with adding tabId to all of the APIs.

2. tabID should never be NULL/0/unknown without other hints

Many addons actually do care about browser-generated requests such as
autocomplete, search bar requests, and other addon requests, and would
like to be able to recognize them as such.

For developer sanity and also for debugging, different parts of the
browser should be given special constant tabIDs, or there should be
special requestInfo request types provided whenever the tabID is null.

I would prefer to have extra request types, if we decide to expose more detailed information.

This will also help find odd edge cases deep in the code where the
tabID is omitted due to oversight or developer ignorance/laziness.
These cases are quite common in Firefox, and they are maddening to
deal with from an extension developer's perspective.

3. The requestID must remain constant across redirect loops

From the API spec, it sounds like this requestID will be constant even
across redirects, but its hard to tell for sure. I could easily see
certain redirect cases causing the implementation to forget to
preserve the requestID. This would be very bad.

Would you please give me an example?

Yes, the requestId is supposed to be constant across redirects.

In Firefox, they actually expose a redirect counter to you through
XPCOM, and you can see how many redirects a URL has left before the
browser gives up. This is pretty important information to have when
you are rewriting URLs arbitrarily.

4. onDOMContentLoaded may need multiple events (or maybe none)

One of the frustrating things about Firefox is being able to intercept
page load at points that make sense to an addon developer as opposed
to a browser implementer. In terms of the DOM, most addon developers
care about 3 points: the DOM is not yet built but you can now insert
your script and/or objects; the DOM is fully built but absolutely no
page scripts have run yet; and the DOM is built and all DOM loaded
events have been dispatched to page script.  However, Firefox exposes
about a dozen different events here, none of which clearly map to this
simple model in terms of DOM events.

It seems like you are talking about "run_at" property of "content_scripts" in manifest?
 

Maybe this is the wrong API set for any DOM-related events though, and
these types of events are only relevant to content scripts who
actually manipulate the DOM. Or maybe all three events should be
present in both places. Something to consider.

I think onDOMContentLoaded is still meaningful. If you were building an extension that monitored performance, you'd be interested in knowing when the document finished loading and document+subresources finished loading.
 

5. webNavigation.onBeforeNavigate should always fire before
webRequest.onBeforeRequest.

Yes, that is specified in the design doc.

 

For many addons, it is very important to be able to tell if a request
was due to navigation, or due to some automatic load condition.

What do you mean by "automatic load condition"?

This is particularly problematic with async events though. If
onBeforeNavigate fires, but it is possible that an extension does so
much work there that it doesn't finish before onBeforeRequest is
delivered, this will cause unexpected behavior in addons that depend
on this knowledge.

If this cannot be avoided due to the event+process model, there should
be a flag added to the extraInfo object in onBeforeRequest to indicate
the request was generated due to a navigation. If this is already what
the "main_frame" request type is supposed to provide, then please
ignore this point entirely :)

The "main_frame" ("sub_frame") request type, indicates the request will be used as page in a mainframe (subframe), which is resulted from a mainframe (subframe) navigation.

6. Host "*" should be allowed in the permissions.

The design doc mentioned that host match patterns (such as http://*/*) are allowed in the permissions.

--
Best regards,
Yuzhu Shen.

[Mike Perry]

On Aug 12, 6:42 pm, Erik Kay <erik...@chromium.org> wrote:
> Hi Mike.  Thanks for the detailed feedback.  I'll let others respond to the
> rest of your points, but I wanted to talk about one in particular.
>

> On Mon, Aug 9, 2010 at 4:55 PM, Mike Perry <mikeperry.unu...@gmail.com>wrote:
>
> > 2. tabID should never be NULL/0/unknown without other hints
> >
> > Many addons actually do care about browser-generated requests such as
> > autocomplete, search bar requests, and other addon requests, and would
> > like to be able to recognize them as such.
>
> I don't think it's a safe assumption that this API should allow modification
> of non-page-generated requests.  It exposes the browser to a range of
> attacks that it isn't open to today, so I think this is too dangerous.  It's
> possible that we could provide specific features to accomplish your use
> cases in a safer way though (for example, maybe we could allow blocking but
> not modification).  Could you give us an idea of what you'd like to do in
> these cases?

In the Firefox case, Torbutton needs to modify the search toolbar
queries to strip off identifying information during Tor usage, which
includes OS distribution and version, Firefox version, and locale. We
also need to redirect update pings and Torbutton updates through the
Tor proxy, since the data retention policy for addons.mozilla.org is
to retain full logs with IP addresses indefinitely. HTTPS Everywhere
also needs to redirect the search toolbar queries and query completion
to go over https, when applicable.

If I understand right, the main risk should be changing the origin of
https-secured update requests generated by the browser.. Though I
suppose that safebrowsing url updates are authenticated by HMAC key
still, and not https origin.. Are there other concerns?

[Mike Perry]

On Aug 12, 2:29 pm, Yuzhu Shen <yzs...@google.com> wrote:

> > 3. The requestID must remain constant across redirect loops
>
> > From the API spec, it sounds like this requestID will be constant even
> > across redirects, but its hard to tell for sure. I could easily see
> > certain redirect cases causing the implementation to forget to
> > preserve the requestID. This would be very bad.
>
> Would you please give me an example?

It could just be the Byzantine nature of the Firefox codebase, but the
channel behavior for different redirects sometimes behaves slightly
differently. I haven't dug far down deep enough to check exactly what
causes it, but I know some types of favicon redirects are one source
of trouble. It would be a good idea to make sure you have unit tests
or other testing code coverage for all the 30x codes going to and from
different schemes and origins.

> Yes, the requestId is supposed to be constant across redirects.

Ok, cool. It may be not a huge difficulty to do this in Chrome, but it
is something I'd keep an eye on in testing coverage or code review.

> > 4. onDOMContentLoaded may need multiple events (or maybe none)
>
> > One of the frustrating things about Firefox is being able to intercept
> > page load at points that make sense to an addon developer as opposed
> > to a browser implementer. In terms of the DOM, most addon developers
> > care about 3 points: the DOM is not yet built but you can now insert
> > your script and/or objects; the DOM is fully built but absolutely no
> > page scripts have run yet; and the DOM is built and all DOM loaded
> > events have been dispatched to page script.  However, Firefox exposes
> > about a dozen different events here, none of which clearly map to this
> > simple model in terms of DOM events.
>
> It seems like you are talking about "run_at" property of "content_scripts"
> in manifest?

Yes. However that documentation is not 100% clear. I was planning on
writing a test extension to see if "document_start" would allow me to
wrap window.Date, window.screen, window.plugins and so on with my own
scripts, or if I would need to wait to "document_end". It's also not
100% clear if any page scripts would be able to run between
"document_start" and "document_end" to subvert my protections.

> > Maybe this is the wrong API set for any DOM-related events though, and
> > these types of events are only relevant to content scripts who
> > actually manipulate the DOM. Or maybe all three events should be
> > present in both places. Something to consider.
>
> I think onDOMContentLoaded is still meaningful. If you were building an
> extension that monitored performance, you'd be interested in knowing when
> the document finished loading and document+subresources finished loading.

Ok, sounds good. You might want to consider mirroring the content
script instrumentation points in that case. People may want
performance stats on those points as well, esp if they are wondering
about the performance impact of their content script addons.

> For many addons, it is very important to be able to tell if a request was due to navigation, or due to some automatic load condition.
>
> What do you mean by "automatic load condition"?

Browser updates, addon updates, other addon loads, search suggestions,
page javascript..

> > This is particularly problematic with async events though. If
> > onBeforeNavigate fires, but it is possible that an extension does so
> > much work there that it doesn't finish before onBeforeRequest is
> > delivered, this will cause unexpected behavior in addons that depend
> > on this knowledge.
>
> > If this cannot be avoided due to the event+process model, there should
> > be a flag added to the extraInfo object in onBeforeRequest to indicate
> > the request was generated due to a navigation. If this is already what
> > the "main_frame" request type is supposed to provide, then please
> > ignore this point entirely :)
>
> The "main_frame" ("sub_frame") request type, indicates the request will be
> used as page in a mainframe (subframe), which is resulted from a mainframe
> (subframe) navigation.

Ok. Sounds like this should be sufficient to not require subscription
to both onBeforeNavigate and onBeforeRequest.

[Erik Kay]

Any https-secured requests generated by the browser (there are a couple of them).  As you said, safebrowsing doesn't fall into this category.  In addition to changing the origin, it would also be problematic if the extension could modify the return payload in these cases..

Erik