1468434 - test_cache_padding.html fails on android if the serialized ChannelInfo is too large (~6000 bytes)

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

In attempting to fix bug 1465562, I introduced code that caused test_cache_padding.html to fail on android. Upon investigating, I found that a) this test fails even without the patch on my linux machine and b) if I do nothing more than change the size of the serialization of some low-level gecko data structures (in particular, the nsISSLStatus for https channels), the test fails on android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7442daa71457c862543af1da493a21101bb64043

To me this indicates that either the test is already faulty or perhaps the underlying implementation is not quite doing the right thing (I feel like web-facing APIs shouldn't be affected by gecko-private state).

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Tom, it looks like you wrote the original implementation and test - thoughts?

Comment 2

[Tom Tung [:tt, :ttung]]

(In reply to [:keeler] (use needinfo) from comment #0)
> In attempting to fix bug 1465562, I introduced code that caused
> test_cache_padding.html to fail on android. Upon investigating, I found that
> a) this test fails even without the patch on my linux machine and b) if I do

The implementation was implemented on my linux desktop and also verified on try server, so I reckon it shouldn't fail without any changes at least on linux.

> nothing more than change the size of the serialization of some low-level
> gecko data structures (in particular, the nsISSLStatus for https channels),
> the test fails on android:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7442daa71457c862543af1da493a21101bb64043
> 
> To me this indicates that either the test is already faulty or perhaps the
> underlying implementation is not quite doing the right thing (I feel like
> web-facing APIs shouldn't be affected by gecko-private state).

This test [1] checks whether the padding is generated correctly by calculating size for a response. I suspect that the patch [2] for investigating modifies sizes for responses randomly so that it confuses the test and makes it fail.

[1] https://searchfox.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_padding.html#137-142
[2] https://hg.mozilla.org/try/rev/cdc58176ddba9c9a7af1dddaaa3103b0baee9262#l1.40

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

> (I feel like web-facing APIs shouldn't be affected by gecko-private state).

Note, the size taken on disk can totally be implementation dependent and will depend on gecko details.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Tom Tung [:tt] (not receiving bugmail, please ni me) from comment #2)
> (In reply to [:keeler] (use needinfo) from comment #0)
> > In attempting to fix bug 1465562, I introduced code that caused
> > test_cache_padding.html to fail on android. Upon investigating, I found that
> > a) this test fails even without the patch on my linux machine and b) if I do
> 
> The implementation was implemented on my linux desktop and also verified on
> try server, so I reckon it shouldn't fail without any changes at least on
> linux.

And yet, if I run `./mach mochitest dom/cache/test/mochitest/test_cache_padding.html`, I get these results:

Unexpected Results
------------------
dom/cache/test/mochitest/test_cache_padding.html
  FAIL The opaque response should be removed by caches.delete() and cache.delete()
    @dom/cache/test/mochitest/test_cache_padding.html:189:3


> > nothing more than change the size of the serialization of some low-level
> > gecko data structures (in particular, the nsISSLStatus for https channels),
> > the test fails on android:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=7442daa71457c862543af1da493a21101bb64043
> > 
> > To me this indicates that either the test is already faulty or perhaps the
> > underlying implementation is not quite doing the right thing (I feel like
> > web-facing APIs shouldn't be affected by gecko-private state).
> 
> This test [1] checks whether the padding is generated correctly by
> calculating size for a response. I suspect that the patch [2] for
> investigating modifies sizes for responses randomly so that it confuses the
> test and makes it fail.
> 
> [1]
> https://searchfox.org/mozilla-central/source/dom/cache/test/mochitest/
> test_cache_padding.html#137-142
> [2]
> https://hg.mozilla.org/try/rev/cdc58176ddba9c9a7af1dddaaa3103b0baee9262#l1.40

Well, that's rather the point. If the underlying implementation changes the serialized size of internal information, this test may fail (although why only on android so far is unclear). For engineering purposes, I think it's untenable for DOM to mandate that the serialization of an underlying implementation detail never change. Consequently, either this implementation needs to change how it calculates the size of the data it's dealing with or this test needs to change.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

David, no one is saying that the serialization cannot change.  This test can be updated to reflect a new size expectation.  But we must store the serialized security information state for responses in some way.  So far the only way we have to do that is with the serialized format provided.  That serialization format can change, but we must be able to safely read old formats and upgrade to the new format.

I think what Tom is trying to say is that our expectation is that the format of the serialization will not randomly change while the browser is still running.  You randomization patch breaks the (reasonable IMO) assumption that serialization is predictable and reproducible.

Of course its possible to add randomization to serialization, but I don't think its fair for you to say people should have expected this to happen when generally serialization is not random in the vast majority of systems.  If you want to add this, then I think you need to fix the test to expect the new behavior you are adding.

And to be clear, this test is not trying to verify the size of the your serialized data.  Its trying to verify that we properly add other randomization noise to the opaque responses in cache API.  Since this is all lumped together its unavoidable that it must depend on the size of all the data stored in cache API.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

The original example was a poor one - here's a much better one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08b0ef396090609e0b3a17236603501634bd6bc1&selectedJob=183236926
https://taskcluster-artifacts.net/dm8g1Q_FQU-Qyb_wLwkurQ/0/public/test_info//logcat-emulator-5554.log (search for test_cache_padding.html)

That patch ensures the size of the serialized ChannelInfo is always 6404 bytes and does nothing else (except some logging). So it seems the cache padding implementation doesn't behave as expected on android if the serialized size is more than a few thousand bytes (the size was originally 2644 bytes).

Comment 7

[Tom Tung [:tt, :ttung]]

(In reply to [:keeler] (use needinfo) from comment #6)
> The original example was a poor one - here's a much better one:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=08b0ef396090609e0b3a17236603501634bd6bc1&selectedJob=1
> 83236926
> https://taskcluster-artifacts.net/dm8g1Q_FQU-Qyb_wLwkurQ/0/public/test_info//
> logcat-emulator-5554.log (search for test_cache_padding.html)
> 
> That patch ensures the size of the serialized ChannelInfo is always 6404
> bytes and does nothing else (except some logging). So it seems the cache
> padding implementation doesn't behave as expected on android if the
> serialized size is more than a few thousand bytes (the size was originally
> 2644 bytes).

Well, it does indicate what I suspected is wrong (to be not related to randomization).

The implementation wasn't specific to any platform, so it's not clear to me why it only fails on android (Though it fails on linux based on comment 4, the error message for that seems that it's not the same issue for android).

I guess the test need to be updated, because the current approach to verify the padding is based on measuring the overall size for a response which is not really direct to the size of padding. Perhaps, I should've implemented a internal method to get the size of padding directly and use it to check whether the size meets expectation or not in the test.