Skip to content
Snippets Groups Projects
  1. Jan 10, 2018
  2. Jan 09, 2018
  3. Jan 03, 2018
  4. Jan 02, 2018
    • Brendan Zagaeski's avatar
      [System]: Ensure WebConnection calls nstream.EndWrite() · 4f8a3184
      Brendan Zagaeski authored
      Also ensure that WebConnection calls nstream.EndRead().
      
      Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=61002
      
      This bug is a regression in HTTPS web request cancellation behavior
      first introduced on the 2017-06 branch by
      https://github.com/mono/mono/commit/1b5e0f73316ee7b83c8947bc738015443fabd7af.
      It is in fact resolved already for the master branch by the refactoring
      in https://github.com/mono/mono/pull/6125.  But since that refactoring
      is too large to backport and since the master branch won't be integrated
      into a release for a while longer, it is worth adding a separate smaller
      fix for the older active development branches.
      
      This proposed fix is intentionally conservative in how it modifies
      `WebConnection.EndWrite()` and `WebConnection.EndRead()`.  It makes sure
      to keep precisely the same return values and exceptions as before.
      
      The fix also makes a small change to the `Close()` method to avoid
      setting `nstream = null`.  This allows any pending `EndWrite()`,
      `EndRead()`, or `ReadDone()` callbacks to use the `nstream` field even
      after `Close()` has run.  This shouldn't cause any new garbage
      collection issues because the closed `WebConnection` object is not
      expected to live much longer than the `NetworkStream` object referenced
      by `nstream`.
      
      Example of the problem for canceling `HttpClient.SendAsync()`
      =============================================================
      
      1. `WebConnectionStream.SetHeadersAsync()` calls
         `WebConnection.BeginWrite()`, passing in an anonymous callback method
         that will later call `WebConnection.EndWrite()` when the asynchronous
         write operation completes [1].
      
      2. When the `CancellationToken` for the call to `HttpClient.SendAsync()`
         is notified about the cancellation, it calls `HttpWebRequest.Abort()`
         [2], which calls `WebConnection.Abort()`, which calls
         `WebConnection.Close()`.
      
      3. `WebConnection.Close()` calls `Close()` on both the
         `MobileAuthenticatedStream` and the network `Socket` and nulls out
         the `WebConnection`'s field references to those objects [3].
         `MobileAuthenticatedStream.Close()` in turn calls
         `MobileAuthenticatedStream.Dispose()`, which sets the `lastException`
         field to an `ObjectDisposedException`.  If the asynchronous write
         operation from step 1 is still in progress at this point, it will
         throw the `ObjectDisposedException`.
      
      4. When `MobileAuthenticatedStream.StartOperation()` throws the
         `ObjectDisposedException` (thus completing the running Task), the
         anonymous callback method from step 1 gets invoked, and it calls
         `WebConnection.EndWrite()`.
      
      5. The problem is that now `WebConnection.EndWrite()` fails to call
         `MobileAuthenticatedStream.EndWrite()`.  There are 2 parts of the
         problem:
      
         (a) `WebConnection.EndWrite()` always returns _before_ calling
         `MobileAuthenticatedStream.EndWrite()` [4] because the call to
         `Abort()` at step 2 has set the request status to canceled [5].
      
         (b) Even if the cancellation status check weren't there, the attempt
         to call `MobileAuthenticatedStream.EndWrite()` still wouldn't work
         because `Close()` has set the reference to the stream to `null` at
         step 3.
      
      The proposed fix addresses (a) by rearranging `WebConnection.EndWrite()`
      and `WebConnection.EndRead()` so that they _always_ call the
      corresponding `MobileAuthenticatedStream` methods.  It addresses (b) by
      removing the line in `Close()` that sets the `nstream` field to `null`.
      
      How did mono/2017-06 commit 1b5e0f73 introduce this problem?
      ===========================================================
      
      Commit 1b5e0f73 uses Tasks and the `TaskToApm()` helper method
      _correctly_, so by itself it's OK.  The problem is that the older
      `WebConnection` methods already had underlying limitations _before_
      commit 1b5e0f73.  In the Asynchronous Programming Model, "for each call
      to BeginOperationName, the application should also call EndOperationName
      to get the results of the operation" [6].  But `WebConnection` did not
      strictly follow this rule.  For example, `WebConnection.EndWrite()` did
      _not_ always call `MobileAuthenticatedStream.EndWrite()`.  This
      imprecise behavior worked until now because unlike unobserved exceptions
      in Tasks, "unobserved" exceptions in AsyncResult objects don't trigger
      any behaviors when the objects are finalized.  (The exceptions are just
      discarded.)  But now that `MobileAuthenticatedStream.BeginWrite()` uses
      Tasks behind the scenes, `WebConnection` needs to be stricter about
      following the rules.
      
      Testing the proposed fix against old bugs from `git blame`
      ==========================================================
      
      - Commit 613c2a91
      
        https://bugzilla.xamarin.com/show_bug.cgi?id=31507
      
        Results: Verified still fixed.  I was able to reproduce the original
        behavior by intentionally removing the old fix.  I then put the old
        fix back, added the new proposed changes, and verified that the test
        case still produced the good behavior (only task cancellation
        messages).
      
      - Commit 7584df6c
      
        Neither of the bugs mentioned in the commit message for 7584df6c
        appears to be related to the `Data.request` check.  Since that check
        is the only part of the commit touched by the new proposed changes,
        neither of the bugs should be affected.  But just to be thorough, I
        quickly ran through the test cases associated with those bugs.
      
        https://bugzilla.novell.com/show_bug.cgi?id=515931
      
        Results: OK.  I tried the test case from
        https://bugzilla.novell.com/show_bug.cgi?id=515097 with
        `AllowWriteStreamBuffering` set to `true` or `false`.  The write
        operation started successfully in both cases, and the call to
        `Abort()` was able to cancel the operation as expected.
      
        https://bugzilla.novell.com/show_bug.cgi?id=510642
      
        Results: OK.  The test case still produced the expected overflow
        exception.
      
      References
      ==========
      
      [1] https://github.com/mono/mono/blob/b293f453027bc075ce636988b383451e5c346347/mcs/class/System/System.Net/WebConnectionStream.cs#L664-L666
      [2] https://github.com/mono/mono/blob/b293f453027bc075ce636988b383451e5c346347/mcs/class/System.Net.Http/System.Net.Http/HttpClientHandler.cs#L347
      [3] https://github.com/mono/mono/blob/b293f453027bc075ce636988b383451e5c346347/mcs/class/System/System.Net/WebConnection.cs#L1131-L1143
      [4] https://github.com/mono/mono/blob/b293f453027bc075ce636988b383451e5c346347/mcs/class/System/System.Net/WebConnection.cs#L1033-L1034
      [5] https://github.com/mono/mono/blob/b293f453027bc075ce636988b383451e5c346347/mcs/class/System/System.Net/WebConnection.cs#L1169
      [6] https://docs.microsoft.com/dotnet/standard/asynchronous-programming-patterns/asynchronous-programming-model-apm
      4f8a3184
  5. Dec 20, 2017
  6. Dec 17, 2017
  7. Dec 16, 2017
    • Marek Habersack's avatar
      [android] Android NDK does not contain API level/platform 12 · 9bd33ec5
      Marek Habersack authored
      Lack of it causes a build error in XA:
      
         CMake Error at [..]mono/external/boringssl/util/android-cmake/android.toolchain.cmake:800 (message):
               Specified Android native API level 'android-12' is not supported by your
      
      The set of minimum API levels in `configure.ac` is incorrect. The current
      NDK (r16) requires minimum API level of 14 for the 32-bit targets and level 21
      for the 64-bit targets.
      9bd33ec5
  8. Dec 14, 2017
  9. Dec 05, 2017
  10. Nov 29, 2017
  11. Nov 28, 2017
  12. Nov 27, 2017
  13. Nov 24, 2017
  14. Nov 23, 2017
  15. Nov 22, 2017
  16. Nov 21, 2017
  17. Nov 20, 2017
    • Chris Hamons's avatar
      efce3b25
    • Aleksey Kliger's avatar
      [loader] Rework get_method_constrained (Fixes #60545) · ce9bf964
      Aleksey Kliger authored
      Instead of using find_method which relies on method name and exact signature
      matching (and thus can't work for variant interfaces), use the vtable and the
      interface offset to find the constrained method.
      
      Terminology:
        base method - a method of the base class to be called
        base class - some general class or interface that declares a method
        constraint class - a more specific class that also implements the method.
        constrained method - the version of the base method in the constraint class.
      
      To find the constrained method given the base method:
      
      - if the base class is a class:
        the constraint class can't be an interface. it's either the base class or
        some subclass of it.  If the base method isn't virtual there's nothing to do
        the constrained method is the same one.
        If the base method is virtual, get its vtable slot and find the corresponding
        method in the constraint class's vtable.
      - if the base class is an interface:
        in this case the constraint class is either another interface or a class.
        if the constraint class is an interface, since interfaces can't have
        implementations, there's nothing to do.
        if the constraint class is a class, then to find the constrained method
        get the interface slot offset of the base method and find the base interface offset
        of the base interface and look in the constrained class vtable.
        (the base interface offset gives the starting vtable slot for the base
        interface in the constraint class vtable; the slot offsets go consecutively
        for each of the methods of the interface).
      
      Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=60545
      ce9bf964
  18. Nov 17, 2017
Loading