Last Comment Bug 722033 - Remove calls to synchronous openCacheEntry in nsHttpChannel
: Remove calls to synchronous openCacheEntry in nsHttpChannel
Status: RESOLVED FIXED
[Snappy:P1]
: main-thread-io, perf
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla14
Assigned To: Michal Novotny (:michal)
:
:
Mentors:
Depends on: 739144 764337
Blocks: CVE-2011-0082 695399 712914 717761 722034
  Show dependency treegraph
 
Reported: 2012-01-28 03:50 PST by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-07-01 17:01 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 (19.34 KB, patch)
2012-02-27 15:34 PST, Michal Novotny (:michal)
no flags Details | Diff | Splinter Review
patch 2 - removes OpenCacheEntry() in OpenOfflineCacheEntryForWriting() (18.40 KB, patch)
2012-02-27 15:39 PST, Michal Novotny (:michal)
no flags Details | Diff | Splinter Review
part1 v2 (20.46 KB, patch)
2012-03-06 03:15 PST, Michal Novotny (:michal)
no flags Details | Diff | Splinter Review
part2 v2 - fixed according to comment #9 (18.15 KB, patch)
2012-03-06 03:16 PST, Michal Novotny (:michal)
no flags Details | Diff | Splinter Review
part1 v3 - the new argument of the asyncOpenCacheEntry() is now optional (14.80 KB, patch)
2012-03-13 17:16 PDT, Michal Novotny (:michal)
honzab.moz: review+
Details | Diff | Splinter Review
part2 v3 (18.14 KB, patch)
2012-03-13 17:17 PDT, Michal Novotny (:michal)
honzab.moz: review+
jaas: superreview+
Details | Diff | Splinter Review
part3 (17.12 KB, patch)
2012-03-13 17:23 PDT, Michal Novotny (:michal)
honzab.moz: review+
Details | Diff | Splinter Review
part3 v2 (17.17 KB, patch)
2012-03-19 11:22 PDT, Michal Novotny (:michal)
no flags Details | Diff | Splinter Review
part3 v3 (17.42 KB, patch)
2012-03-19 12:58 PDT, Michal Novotny (:michal)
jaas: superreview+
Details | Diff | Splinter Review
part1 v4 - fixed typo (14.80 KB, patch)
2012-03-19 17:06 PDT, Michal Novotny (:michal)
jaas: superreview+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-01-28 03:50:21 PST
+++ This bug was initially created as a clone of Bug #695399 +++

Synchronous openCacheEntry takes the cache service lock, and that lock can block on disk I/O, so we cannot call it from the main thread, including anywhere in nsHttpChannel.
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-01-28 03:57:40 PST
This is blocking P1 work in PSM and it is blocking bugs that likely need to be fixed to keep the disk cache enabled on mobile. I am going to take a stab at it.
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-15 02:52:23 PST
Michal, do you have anything that I start testing here? I would like to build my patches on top of this one, even if the changes to the tests are not ready.
Comment 3 Michal Novotny (:michal) 2012-02-20 14:00:45 PST
(In reply to Brian Smith (:bsmith) from comment #2)
> Michal, do you have anything that I start testing here? I would like to
> build my patches on top of this one, even if the changes to the tests are
> not ready.

What exactly do you need for testing? BTW changing tests is not a scope of this bug and will be covered by bug #695399.

So far I have eliminated openCacheEntry() that is needed due to LOAD_BYPASS_LOCAL_CACHE_IF_BUSY flag, but the patch break some xpc-tests and I'm now trying to find out what's wrong.
Comment 4 Michal Novotny (:michal) 2012-02-27 15:34:59 PST
Created attachment 601097 [details] [diff] [review]
patch 1

- adds blocking argument to nsICacheSession::asyncOpenCacheEntry()
- removes OpenCacheEntry() needed due to LOAD_BYPASS_LOCAL_CACHE_IF_BUSY flag
Comment 5 Michal Novotny (:michal) 2012-02-27 15:39:46 PST
Created attachment 601099 [details] [diff] [review]
patch 2 - removes OpenCacheEntry() in OpenOfflineCacheEntryForWriting()

This patch should be applied on the top of patch #601097.
Comment 6 Honza Bambas (:mayhemer) 2012-03-02 15:30:16 PST
I'll publish the reviews probably tomorrow.
Comment 7 Honza Bambas (:mayhemer) 2012-03-03 08:53:21 PST
Has this been pushed to try?
Comment 8 Honza Bambas (:mayhemer) 2012-03-03 09:18:43 PST
Comment on attachment 601097 [details] [diff] [review]
patch 1

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

r=honzab

I think this is correct.

I'd love to find some way to completely avoid duplication of code at http://hg.mozilla.org/mozilla-central/annotate/343ec916dfd5/netwerk/protocol/http/nsHttpChannel.cpp#l249.

::: netwerk/cache/nsCacheService.cpp
@@ +1021,5 @@
>                                                        nsnull);
>  
>          // Don't delete the request if it was queued
> +        if (!(mRequest->IsBlocking() &&
> +            rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION))

May be more readable as:

!mRequest-IsBlocking() ||
rv != NS_ERROR_CACHE_WAIT_FOR_VALIDATION

@@ +1669,5 @@
> +                if (request->IsBlocking()) {
> +                    // async exits - validate, doom, or close will resume
> +                    return rv;
> +                }
> +            } else if (request->IsBlocking()) {

As I understand, a non-blocking request is intended to give up when the cache entry can't be opened (from whatever reason) and to cancel the |open| operation, right?  

However blocking ones are intended to always try to open the cache entry even on WAIT_FOR_VALIDATION (either synchronously through WaitForValidation() or asynchronously via callback), is that so?


I think for simplicity the condition may just be:
if (blocking) {
  if (listener) 
    return rv;

  WaitForValidation();
}

@@ +1777,5 @@
>          rv = gService->ProcessRequest(request, true, result);
>  
>          // delete requests that have completed
> +        if (!(listener && blockingMode &&
> +            (rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION)))

Perhaps the same here (! || ! || !)...

Non-blocking requests are done here according my previous comment, right?


Please also check whether to delete requests in nsCacheService::ProcessPendingRequests also when the request is non-blocking.  I think you should since non-blocking async requests may return NS_ERROR_CACHE_WAIT_FOR_VALIDATION.  I actually think the condition should be the same as are now here.

::: netwerk/cache/nsICacheSession.idl
@@ +81,5 @@
>       * available.
>       */
>      void asyncOpenCacheEntry(in ACString          key,
>                               in nsCacheAccessMode accessRequested,
> +                             in boolean           blockingMode,

Also update the documentation.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2255,5 @@
> +        accessRequested,
> +        !(mLoadFlags & LOAD_BYPASS_LOCAL_CACHE_IF_BUSY),
> +        this);
> +
> +    if (rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION) {

Can you ever get this error here regardless blocking or non-blocking request?  I think the state needed to determine this error result is processed only asynchronously on the cache thread, right?
Comment 9 Honza Bambas (:mayhemer) 2012-03-03 09:19:18 PST
Comment on attachment 601099 [details] [diff] [review]
patch 2 - removes OpenCacheEntry() in OpenOfflineCacheEntryForWriting()

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

If there were anything wrong with the changes then tests should catch it.  I don't want to stare in to the patch too long, it proved to be useless.

r=honzab


However, both reviews are conditioned by an as-much-as-possible-green try run (I'm going to push it right now).

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2258,5 @@
>          LOG(("bypassing local cache since it is busy\n"));
>          rv = NS_ERROR_NOT_AVAILABLE;
>      }
>  
> +    if (NS_SUCCEEDED(rv)) return NS_OK;

return on a new line please.

@@ +2349,5 @@
>          // access to the cache entry has been denied (because the cache entry
>          // is probably in use by another channel).  Either the cache is being
>          // read from (we're offline) or it's being updated elsewhere.
>          return NS_OK;
>      }

Can we ever get this result from AsyncOpenCacheEntry ?

@@ +2354,5 @@
>  
> +    if (NS_SUCCEEDED(rv))
> +        return NS_OK;
> +
> +    return rv;

Just return rv?

@@ +4890,5 @@
> +                return NS_OK;
> +            }
> +
> +            if (NS_FAILED(rv))
> +                return rv;

Could we potentially do this in parallel?  (In a follow-up, though a helper class probably)
Comment 10 Honza Bambas (:mayhemer) 2012-03-03 09:27:26 PST
https://tbpl.mozilla.org/?tree=Try&rev=f8097830c2c0
Comment 11 Honza Bambas (:mayhemer) 2012-03-03 12:03:32 PST
Comment on attachment 601099 [details] [diff] [review]
patch 2 - removes OpenCacheEntry() in OpenOfflineCacheEntryForWriting()

There is a consistent M(oth) tests failure.
Comment 12 Honza Bambas (:mayhemer) 2012-03-03 12:04:06 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=9799939&tree=Try
Comment 13 Michal Novotny (:michal) 2012-03-06 03:15:04 PST
Created attachment 603215 [details] [diff] [review]
part1 v2

(In reply to Honza Bambas (:mayhemer) from comment #8)
> I'd love to find some way to completely avoid duplication of code at
> http://hg.mozilla.org/mozilla-central/annotate/343ec916dfd5/netwerk/protocol/
> http/nsHttpChannel.cpp#l249.

I agree, but I would prefer to do this in a follow-up bug.


> >          // Don't delete the request if it was queued
> > +        if (!(mRequest->IsBlocking() &&
> > +            rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION))
> 
> May be more readable as:
> 
> !mRequest-IsBlocking() ||
> rv != NS_ERROR_CACHE_WAIT_FOR_VALIDATION

I think that negation of conjunction is easier to understand than disjunction of negation. But if others have the same opinion as you, I can change it.


> As I understand, a non-blocking request is intended to give up when the
> cache entry can't be opened (from whatever reason) and to cancel the |open|
> operation, right?  
> 
> However blocking ones are intended to always try to open the cache entry
> even on WAIT_FOR_VALIDATION (either synchronously through
> WaitForValidation() or asynchronously via callback), is that so?

Correct.


> I think for simplicity the condition may just be:
> if (blocking) {
>   if (listener) 
>     return rv;
> 
>   WaitForValidation();
> }

Right, this is easier to read. Fixed.


> @@ +1777,5 @@
> >          rv = gService->ProcessRequest(request, true, result);
> >  
> >          // delete requests that have completed
> > +        if (!(listener && blockingMode &&
> > +            (rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION)))
> 
> Perhaps the same here (! || ! || !)...

The same as above...

 
> Non-blocking requests are done here according my previous comment, right?
> 
> 
> Please also check whether to delete requests in
> nsCacheService::ProcessPendingRequests also when the request is
> non-blocking.  I think you should since non-blocking async requests may
> return NS_ERROR_CACHE_WAIT_FOR_VALIDATION.  I actually think the condition
> should be the same as are now here.

I don't think so. We should never end up processing a non-blocking request in nsCacheService::ProcessPendingRequests() since it is never queued.


> ::: netwerk/cache/nsICacheSession.idl
> @@ +81,5 @@
> >       * available.
> >       */
> >      void asyncOpenCacheEntry(in ACString          key,
> >                               in nsCacheAccessMode accessRequested,
> > +                             in boolean           blockingMode,
> 
> Also update the documentation.

I've updated the description in idl and I'll update MDN after landing this patch.


> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +2255,5 @@
> > +        accessRequested,
> > +        !(mLoadFlags & LOAD_BYPASS_LOCAL_CACHE_IF_BUSY),
> > +        this);
> > +
> > +    if (rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION) {
> 
> Can you ever get this error here regardless blocking or non-blocking
> request?  I think the state needed to determine this error result is
> processed only asynchronously on the cache thread, right?

Right, AsyncOpenCacheEntry() never returns NS_ERROR_CACHE_WAIT_FOR_VALIDATION. Fixed.


> There is a consistent M(oth) tests failure.

Fixed.
Comment 14 Michal Novotny (:michal) 2012-03-06 03:16:49 PST
Created attachment 603216 [details] [diff] [review]
part2 v2 - fixed according to comment #9

(In reply to Honza Bambas (:mayhemer) from comment #9)
> @@ +4890,5 @@
> > +                return NS_OK;
> > +            }
> > +
> > +            if (NS_FAILED(rv))
> > +                return rv;
> 
> Could we potentially do this in parallel?  (In a follow-up, though a helper
> class probably)

Do you mean to do OpenOfflineCacheEntryForWriting() and OpenNormalCacheEntry() in parallel? It should be possible, but it is definitely out of scope of this bug.
Comment 15 Michal Novotny (:michal) 2012-03-06 03:20:09 PST
pushed to try server

https://tbpl.mozilla.org/?tree=Try&rev=861d85480e00
Comment 16 Honza Bambas (:mayhemer) 2012-03-06 12:43:06 PST
(In reply to Michal Novotny (:michal) from comment #13)
> I agree, but I would prefer to do this in a follow-up bug.

Definitely.  Can you please file it?

> I think that negation of conjunction is easier to understand than
> disjunction of negation. But if others have the same opinion as you, I can
> change it.

I perceive simpler to see that when just one condition doesn't apply and the code executes.  But up to you.

> I don't think so. We should never end up processing a non-blocking request
> in nsCacheService::ProcessPendingRequests() since it is never queued.

Then it's OK.  Maybe worth a comment?

(In reply to Michal Novotny (:michal) from comment #14)
> Do you mean to do OpenOfflineCacheEntryForWriting() and
> OpenNormalCacheEntry() in parallel? It should be possible, but it is
> definitely out of scope of this bug.

I don't think this is worth to fix, actually...  It may just have a minor affect on background offline cache loads.
Comment 17 Honza Bambas (:mayhemer) 2012-03-06 12:48:07 PST
Comment on attachment 603215 [details] [diff] [review]
part1 v2

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

As I understand today discussion during the meeting, you will update the IDL ones more, right?  This patch looks good, but I think I should review the final one ones more.

So, preliminary r+, I'll take a quick look at the final patch.
Comment 18 Honza Bambas (:mayhemer) 2012-03-06 12:53:24 PST
Comment on attachment 603216 [details] [diff] [review]
part2 v2 - fixed according to comment #9

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

Looks good, r+, but the same as the previous patch.
Comment 19 Michal Novotny (:michal) 2012-03-13 17:16:08 PDT
Created attachment 605603 [details] [diff] [review]
part1 v3 - the new argument of the asyncOpenCacheEntry() is now optional
Comment 20 Michal Novotny (:michal) 2012-03-13 17:17:33 PDT
Created attachment 605604 [details] [diff] [review]
part2 v3
Comment 21 Michal Novotny (:michal) 2012-03-13 17:23:03 PDT
Created attachment 605605 [details] [diff] [review]
part3

This patch removes the last openCacheEntry() in nsHttpChannel that is used to find and invalidate an entry.
Comment 22 Michal Novotny (:michal) 2012-03-13 17:24:34 PDT
pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=db932be8e043
Comment 23 Michal Novotny (:michal) 2012-03-13 17:54:09 PDT
(In reply to Honza Bambas (:mayhemer) from comment #16)
> (In reply to Michal Novotny (:michal) from comment #13)
> > I agree, but I would prefer to do this in a follow-up bug.
> 
> Definitely.  Can you please file it?

Bug #735538
Comment 24 Honza Bambas (:mayhemer) 2012-03-14 17:42:09 PDT
Comment on attachment 605603 [details] [diff] [review]
part1 v3 - the new argument of the asyncOpenCacheEntry() is now optional

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

r=honzab

::: netwerk/cache/nsICacheSession.idl
@@ +77,5 @@
>  
>      /**
> +     * Asynchronous cache access. Does not block the calling thread. Instead,
> +     * the listener will be notified when the descriptor is available. If
> +     * 'noWait' is set to true, the listener will be notified immediately with

s/will be/is/ I think..
Comment 25 Honza Bambas (:mayhemer) 2012-03-14 17:52:04 PDT
Comment on attachment 605604 [details] [diff] [review]
part2 v3

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

r=honzab
Comment 26 Honza Bambas (:mayhemer) 2012-03-14 18:18:17 PDT
Comment on attachment 605605 [details] [diff] [review]
part3

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

r=honzab with comments addressed and after this passes try.

::: netwerk/cache/nsCacheService.cpp
@@ +1053,5 @@
> +        return NS_OK;
> +    }
> +
> +private:
> +    nsICacheListener *mListener;

nsCOMPtr, see more bellow.

@@ +1094,5 @@
> +        }
> +
> +        if (entry) {
> +            status = NS_OK;
> +            nsCacheService::gService->DoomEntry_Internal(entry, foundActive);

Can this fail?

@@ +1099,5 @@
> +        }
> +
> +        if (mListener) {
> +            mThread->Dispatch(new nsNotifyDoomListener(mListener, status),
> +                              NS_DISPATCH_NORMAL);

nsRefPtr for nsNotifyDoomListener object please.

@@ +1110,5 @@
> +
> +private:
> +    nsCString             mKey;
> +    nsCacheStoragePolicy  mStoragePolicy;
> +    nsICacheListener     *mListener;

We leak the listener when dispatch of this event fails.  This has to be nsCOMPtr.  You can swap it to nsNotifyDoomListener event with dont_AddRef and forget() this one.

@@ +1111,5 @@
> +private:
> +    nsCString             mKey;
> +    nsCacheStoragePolicy  mStoragePolicy;
> +    nsICacheListener     *mListener;
> +    nsCOMPtr<nsIThread>   mThread;

Would nsIEventTarget be enough here?

@@ +1442,5 @@
> +
> +    if (!gService->mInitialized)
> +        return NS_ERROR_NOT_INITIALIZED;
> +
> +    return DispatchToCacheIOThread(new nsDoomEvent(session, key, listener));

Please preserve this style of dispatching:

http://hg.mozilla.org/mozilla-central/annotate/78e56fd22f2a/netwerk/cache/nsCacheService.cpp#l1762

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5213,5 @@
>  
>      if (NS_FAILED(rv))
>          return;
>  
> +    session->DoomEntry(key, nsnull);

Shouldn't DoInvalidateCacheEntry be renamed to DoomCacheEntry or so?

::: netwerk/test/unit/test_doomentry.js
@@ +18,5 @@
> +  return _CSvc = Cc["@mozilla.org/network/cache-service;1"].
> +                 getService(Ci.nsICacheService);
> +}
> +
> +function get_ostream(key, asFile, append, callback)

Hint: objects should be UpperCamelCase to be easily recognized from functions.

@@ +156,5 @@
> +
> +function check_doom4(status)
> +{
> +  do_check_eq(status, Cr.NS_ERROR_NOT_AVAILABLE);
> +  do_test_finished();

Should you rather clear the cache here?
Comment 27 Michal Novotny (:michal) 2012-03-19 11:22:38 PDT
Created attachment 607223 [details] [diff] [review]
part3 v2

(In reply to Honza Bambas (:mayhemer) from comment #26)
> ::: netwerk/cache/nsCacheService.cpp
> > +private:
> > +    nsICacheListener *mListener;
> 
> nsCOMPtr, see more bellow.
> 
>
> > +    nsICacheListener     *mListener;
> 
> We leak the listener when dispatch of this event fails.  This has to be
> nsCOMPtr.  You can swap it to nsNotifyDoomListener event with dont_AddRef
> and forget() this one.

The leak in case of failure is intentional since we need to release the listener on the callers thread. If we can't post the event to the correct thread how we could release the reference on the same thread? BTW we do the same in nsCacheService::NotifyListener().


> @@ +1111,5 @@
> > +private:
> > +    nsCString             mKey;
> > +    nsCacheStoragePolicy  mStoragePolicy;
> > +    nsICacheListener     *mListener;
> > +    nsCOMPtr<nsIThread>   mThread;
> 
> Would nsIEventTarget be enough here?

It would, but what's the benefit? do_GetCurrentThread returns already_AddRefed<nsIThread>.


> > +            mThread->Dispatch(new nsNotifyDoomListener(mListener, status),
> > +                              NS_DISPATCH_NORMAL);
> 
> nsRefPtr for nsNotifyDoomListener object please.
> 
>
> > +    return DispatchToCacheIOThread(new nsDoomEvent(session, key, listener));
> 
> Please preserve this style of dispatching:
> 
> http://hg.mozilla.org/mozilla-central/annotate/78e56fd22f2a/netwerk/cache/
> nsCacheService.cpp#l1762

It's actually a relict from times when new could fail. And this style is also present in the cache code, e.g. http://hg.mozilla.org/mozilla-central/annotate/78e56fd22f2a/netwerk/cache/nsDiskCacheDevice.cpp#l1141.


> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5213,5 @@
> >  
> >      if (NS_FAILED(rv))
> >          return;
> >  
> > +    session->DoomEntry(key, nsnull);
> 
> Shouldn't DoInvalidateCacheEntry be renamed to DoomCacheEntry or so?

I don't think it's necessary. Both dooming the entry as well as setting expiration time to 0 do invalidate the entry.


> ::: netwerk/test/unit/test_doomentry.js
> @@ +18,5 @@
> > +  return _CSvc = Cc["@mozilla.org/network/cache-service;1"].
> > +                 getService(Ci.nsICacheService);
> > +}
> > +
> > +function get_ostream(key, asFile, append, callback)
> 
> Hint: objects should be UpperCamelCase to be easily recognized from
> functions.

Renamed.


> @@ +156,5 @@
> > +
> > +function check_doom4(status)
> > +{
> > +  do_check_eq(status, Cr.NS_ERROR_NOT_AVAILABLE);
> > +  do_test_finished();
> 
> Should you rather clear the cache here?

A common rule is to clean the cache before running the test rather than rely on cleanup performed by previous test.
Comment 28 Honza Bambas (:mayhemer) 2012-03-19 12:02:07 PDT
(In reply to Michal Novotny (:michal) from comment #27)
> The leak in case of failure is intentional ... BTW we do the
> same in nsCacheService::NotifyListener().

Ah, I wasn't aware.  Maybe just add a comment.


I also agree with the rest.  Thanks.
Comment 29 Michal Novotny (:michal) 2012-03-19 12:58:27 PDT
Created attachment 607269 [details] [diff] [review]
part3 v3

(In reply to Honza Bambas (:mayhemer) from comment #26)
> > +        if (entry) {
> > +            status = NS_OK;
> > +            nsCacheService::gService->DoomEntry_Internal(entry, foundActive);
> 
> Can this fail?

I forgot to answer this question. DoomEntry_Internal() returns always NS_OK and I don't think that we'll change this behavior in future.

(In reply to Honza Bambas (:mayhemer) from comment #28)
> (In reply to Michal Novotny (:michal) from comment #27)
> > The leak in case of failure is intentional ... BTW we do the
> > same in nsCacheService::NotifyListener().
> 
> Ah, I wasn't aware.  Maybe just add a comment.

Comment added.

Pushed to tryserver again because of a strange failure in the last try:
https://tbpl.mozilla.org/?tree=Try&rev=5932f13b5eb4
Comment 30 Michal Novotny (:michal) 2012-03-19 17:06:29 PDT
Created attachment 607377 [details] [diff] [review]
part1 v4 - fixed typo

not sure if this need a sr
Comment 33 Florian Quèze [:florian] [:flo] 2012-03-23 08:23:24 PDT
Fix for the comm-central bustage: http://hg.mozilla.org/comm-central/rev/d2f3b160dd8b

David (cc'ed) may want to review this change after the fact.

Note You need to log in before you can comment on or make changes to this bug.