Fixed
Status Update
No update yet.
Comments
to...@gmail.com
Correction:
So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to
should be
So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to, and sometimes neglects to send a ZLP when it should
So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to
should be
So, the end result is that usb_write() sometimes sends a ZLP when it doesn't need to, and sometimes neglects to send a ZLP when it should
xm...@gmail.com
Note that the CHECK_LE has since been replaced with HandleError(), which causes adbd to not abort...but that stowaway amessage header is being dropped all the same...and so the behavior is pretty much undefined, I imagine. I'm not sure the switch to HandleError was a good idea, since it more or less swept the real problem under the rug.
da...@google.com
good bug report, thanks! i'm assuming this isn't darwin-specific either --- it looks like we have similar logic using masks in the linux and windows backends and in the libusb backend too.
I'm not sure the switch to HandleError was a good idea, since it more or less swept the real problem under the rug.
yeah, i know what you mean, but it's also hard to argue with this logic in the commit message that made that change:
These CHECKs are expected to happen if the client does the wrong thing,
so we probably shouldn't be aborting in adbd.
a CHECK in the client (as you suggested earlier) would probably have been the best idea... postel's law and all that :-)
xm...@gmail.com
Thanks for the quick response.
Indeed, I see similar logic in usb_write() in usb_linux. maxPacketSize is 512 (a power of 2) on my Ubuntu environment, so all good there as far as runtime behavior. It sure is odd that on macOS it's 5120, which happens to be exactly 10x the linux value. Seem suspicious, doesn't it?
Yeah, on the CHECK thing... I guess I see the client and server as a closed system in that the code is developed and distributed together. That said, obviously anyone can write their own adb client/server, so in that respect, I agree with the disagreement :-)
Indeed, I see similar logic in usb_write() in usb_linux. maxPacketSize is 512 (a power of 2) on my Ubuntu environment, so all good there as far as runtime behavior. It sure is odd that on macOS it's 5120, which happens to be exactly 10x the linux value. Seem suspicious, doesn't it?
Yeah, on the CHECK thing... I guess I see the client and server as a closed system in that the code is developed and distributed together. That said, obviously anyone can write their own adb client/server, so in that respect, I agree with the disagreement :-)
ap...@google.com
We've been discussing it here some more and we have a theory:
5120 is likely not really the maxPacketSize on our Macs. It's probably 512. And a burst is probably 10 packets.
Note that usb_osx.cpp calculates the maxPacketSize as follows
if (maxBurst != 0)
// bMaxBurst is the number of additional packets in the burst.
maxPacketSize /= (maxBurst + 1);
If the theory about packet size truly being 512 is correct, then it means the real bug is in GetPipePropertiesV2() returning zero for maxBurst instead of 9. If it returned 9, then maxPacketSize would be 512 (again, a power of 2), and the zero_mask expression would work out just fine. All situations where we've had our adbd abort because of the unexpected stowaway amessage header in a packet are ones where the sent buffer are an integer multiple of 512. None are an integer multiple of 5120.
Still there's no denying that the unchecked assumption of maxPacketSize being a power of 2 is a bug. Changing the code to use modulus operation instead of a bit-mask check would be technically more correct, but wouldn't fix the broken behavior in our case, since ultimately, it appears the maxPacketSize adb is getting is wrong. But if you had an assert in the client/server on macPacketSize being a power of 2, then that would at least have brought the incorrect maxPacketSize issue front and center.
5120 is likely not really the maxPacketSize on our Macs. It's probably 512. And a burst is probably 10 packets.
Note that usb_osx.cpp calculates the maxPacketSize as follows
if (maxBurst != 0)
// bMaxBurst is the number of additional packets in the burst.
maxPacketSize /= (maxBurst + 1);
If the theory about packet size truly being 512 is correct, then it means the real bug is in GetPipePropertiesV2() returning zero for maxBurst instead of 9. If it returned 9, then maxPacketSize would be 512 (again, a power of 2), and the zero_mask expression would work out just fine. All situations where we've had our adbd abort because of the unexpected stowaway amessage header in a packet are ones where the sent buffer are an integer multiple of 512. None are an integer multiple of 5120.
Still there's no denying that the unchecked assumption of maxPacketSize being a power of 2 is a bug. Changing the code to use modulus operation instead of a bit-mask check would be technically more correct, but wouldn't fix the broken behavior in our case, since ultimately, it appears the maxPacketSize adb is getting is wrong. But if you had an assert in the client/server on macPacketSize being a power of 2, then that would at least have brought the incorrect maxPacketSize issue front and center.
da...@google.com
Finally, I forced my macOS server to use 512 for maxPacketSize instead of the suspicious (and almost certainly wrong) 5120...and the daemon no longer aborts because it eventually receives a stowaway amessage header in UsbFfsConnection::ProcessRead(). This gives further credence to our theory that the root cause of our problem is that the macOS adb server isn't querying/calculating the max packet size correctly, at least on our MacBook Pros with Big Sur on them.
xm...@gmail.com
A fix for the issue of getting the wrong max packet size might be to use GetPipePropertiesV2 ( and not GetEndpointProperties) for that value.
From GetEndpointPropertiesV3's documentation:
... The wMaxPacketSize is the BASE maxPacketSize as found in the endpoint descriptor, and has not been multiplied to take into account burst and mult...
Whereas GetPipePropertiesV3 documentation states that it does mention is may be a multiple
> .. current FULL maxPacketSize for this pipe, which includes both the mult and the burst...
So using GetEndpointProperties would avoid the need to compute the actual max packet length which goes wrong.
From GetEndpointPropertiesV3's documentation:
... The wMaxPacketSize is the BASE maxPacketSize as found in the endpoint descriptor, and has not been multiplied to take into account burst and mult...
Whereas GetPipePropertiesV3 documentation states that it does mention is may be a multiple
> .. current FULL maxPacketSize for this pipe, which includes both the mult and the burst...
So using GetEndpointProperties would avoid the need to compute the actual max packet length which goes wrong.
an...@google.com
use GetPipePropertiesV2 ( and not GetEndpointProperties)
the opposite way round, right? we should switch to GetEndpointProperties?
do you have a pointer to the docs? my search-fu failed me :-(
da...@google.com
جهازي مخترق
to...@gmail.com
CL is currently under peer review.
xm...@gmail.com
@jc...@magicleap.com: can you please get these two CLs reviewed?
Description
@Fts4(tokenizer = FtsOptions.TOKENIZER_UNICODE61)
class WorldFts(
var word: String
)
java.lang.IllegalArgumentException: There is no table with name word_fts_content
at androidx.room.InvalidationTracker.addObserver(InvalidationTracker.java:266)
at androidx.room.MultiInstanceInvalidationClient$3.run(MultiInstanceInvalidationClient.java:124)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
at java.lang.Thread.run(Thread.java:764)
It happens when use room Fts4 annotation with enableMultiInstanceInvalidation in room database.