Skip to content

Commit

Permalink
Change Pickle::Write* methods to return void.
Browse files Browse the repository at this point in the history 
In practice, 99% of these methods can't fail. WriteData() can fail...
but only if something passes a negative length. Just CHECK in that
case.

TBR=alokp@chromium.org,mpearson@chromium.org,piman@chromium.org
TBR=rdsmith@chromium.org,rockot@chromium.org

Change-Id: I938b1d1fd4f4ccfd9151f1e954831034fd452b66
Reviewed-on: https://chromium-review.googlesource.com/677023
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Kinuko Yasuda (slow) <kinuko@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503662}
  • Loading branch information
@zetafunction
zetafunction authored and Commit Bot committed Sep 22, 2017
1 parent e95a985 commit 0d89f92
Show file tree
Hide file tree
Showing 49 changed files with 275 additions and 441 deletions.
9 changes: 3 additions & 6 deletions android_webview/browser/aw_contents.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -941,12 +941,9 @@ base::android::ScopedJavaLocalRef<jbyteArray> AwContents::GetOpaqueState(
return ScopedJavaLocalRef<jbyteArray>();

base::Pickle pickle;
if (!WriteToPickle(*web_contents_, &pickle)) {
return ScopedJavaLocalRef<jbyteArray>();
} else {
return base::android::ToJavaByteArray(
env, reinterpret_cast<const uint8_t*>(pickle.data()), pickle.size());
}
WriteToPickle(*web_contents_, &pickle);
return base::android::ToJavaByteArray(
env, reinterpret_cast<const uint8_t*>(pickle.data()), pickle.size());
}

jboolean AwContents::RestoreFromOpaqueState(
Expand Down
82 changes: 26 additions & 56 deletions android_webview/browser/state_serializer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,11 @@ const uint32_t AW_STATE_VERSION = internal::AW_STATE_VERSION_DATA_URL;

} // namespace

bool WriteToPickle(const content::WebContents& web_contents,
void WriteToPickle(const content::WebContents& web_contents,
base::Pickle* pickle) {
DCHECK(pickle);

if (!internal::WriteHeaderToPickle(pickle))
return false;
internal::WriteHeaderToPickle(pickle);

const content::NavigationController& controller =
web_contents.GetController();
Expand All @@ -51,24 +50,17 @@ bool WriteToPickle(const content::WebContents& web_contents,
DCHECK_GE(selected_entry, -1); // -1 is valid
DCHECK_LT(selected_entry, entry_count);

if (!pickle->WriteInt(entry_count))
return false;

if (!pickle->WriteInt(selected_entry))
return false;

pickle->WriteInt(entry_count);
pickle->WriteInt(selected_entry);
for (int i = 0; i < entry_count; ++i) {
if (!internal::WriteNavigationEntryToPickle(*controller.GetEntryAtIndex(i),
pickle))
return false;
internal::WriteNavigationEntryToPickle(*controller.GetEntryAtIndex(i),
pickle);
}

// Please update AW_STATE_VERSION and IsSupportedVersion() if serialization
// format is changed.
// Make sure the serialization format is updated in a backwards compatible
// way.

return true;
}

bool RestoreFromPickle(base::PickleIterator* iterator,
Expand Down Expand Up @@ -135,12 +127,12 @@ bool RestoreFromPickle(base::PickleIterator* iterator,

namespace internal {

bool WriteHeaderToPickle(base::Pickle* pickle) {
return WriteHeaderToPickle(AW_STATE_VERSION, pickle);
void WriteHeaderToPickle(base::Pickle* pickle) {
WriteHeaderToPickle(AW_STATE_VERSION, pickle);
}

bool WriteHeaderToPickle(uint32_t state_version, base::Pickle* pickle) {
return pickle->WriteUInt32(state_version);
void WriteHeaderToPickle(uint32_t state_version, base::Pickle* pickle) {
pickle->WriteUInt32(state_version);
}

uint32_t RestoreHeaderFromPickle(base::PickleIterator* iterator) {
Expand All @@ -160,41 +152,27 @@ bool IsSupportedVersion(uint32_t state_version) {
state_version == internal::AW_STATE_VERSION_DATA_URL;
}

bool WriteNavigationEntryToPickle(const content::NavigationEntry& entry,
void WriteNavigationEntryToPickle(const content::NavigationEntry& entry,
base::Pickle* pickle) {
return WriteNavigationEntryToPickle(AW_STATE_VERSION, entry, pickle);
WriteNavigationEntryToPickle(AW_STATE_VERSION, entry, pickle);
}

bool WriteNavigationEntryToPickle(uint32_t state_version,
void WriteNavigationEntryToPickle(uint32_t state_version,
const content::NavigationEntry& entry,
base::Pickle* pickle) {
DCHECK(IsSupportedVersion(state_version));
if (!pickle->WriteString(entry.GetURL().spec()))
return false;

if (!pickle->WriteString(entry.GetVirtualURL().spec()))
return false;
pickle->WriteString(entry.GetURL().spec());
pickle->WriteString(entry.GetVirtualURL().spec());

const content::Referrer& referrer = entry.GetReferrer();
if (!pickle->WriteString(referrer.url.spec()))
return false;
if (!pickle->WriteInt(static_cast<int>(referrer.policy)))
return false;

if (!pickle->WriteString16(entry.GetTitle()))
return false;
pickle->WriteString(referrer.url.spec());
pickle->WriteInt(static_cast<int>(referrer.policy));

if (!pickle->WriteString(entry.GetPageState().ToEncodedData()))
return false;

if (!pickle->WriteBool(static_cast<int>(entry.GetHasPostData())))
return false;

if (!pickle->WriteString(entry.GetOriginalRequestURL().spec()))
return false;

if (!pickle->WriteString(entry.GetBaseURLForDataURL().spec()))
return false;
pickle->WriteString16(entry.GetTitle());
pickle->WriteString(entry.GetPageState().ToEncodedData());
pickle->WriteBool(static_cast<int>(entry.GetHasPostData()));
pickle->WriteString(entry.GetOriginalRequestURL().spec());
pickle->WriteString(entry.GetBaseURLForDataURL().spec());

if (state_version >= internal::AW_STATE_VERSION_DATA_URL) {
const char* data = nullptr;
Expand All @@ -206,25 +184,17 @@ bool WriteNavigationEntryToPickle(uint32_t state_version,
}
// Even when |entry.GetDataForDataURL()| is null we still need to write a
// zero-length entry to ensure the fields all line up when read back in.
if (!pickle->WriteData(data, size))
return false;
pickle->WriteData(data, size);
}

if (!pickle->WriteBool(static_cast<int>(entry.GetIsOverridingUserAgent())))
return false;

if (!pickle->WriteInt64(entry.GetTimestamp().ToInternalValue()))
return false;

if (!pickle->WriteInt(entry.GetHttpStatusCode()))
return false;
pickle->WriteBool(static_cast<int>(entry.GetIsOverridingUserAgent()));
pickle->WriteInt64(entry.GetTimestamp().ToInternalValue());
pickle->WriteInt(entry.GetHttpStatusCode());

// Please update AW_STATE_VERSION and IsSupportedVersion() if serialization
// format is changed.
// Make sure the serialization format is updated in a backwards compatible
// way.

return true;
}

bool RestoreNavigationEntryFromPickle(base::PickleIterator* iterator,
Expand Down
17 changes: 8 additions & 9 deletions android_webview/browser/state_serializer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ namespace android_webview {
// success.

// Note that |pickle| may be changed even if function returns false.
bool WriteToPickle(const content::WebContents& web_contents,
base::Pickle* pickle) WARN_UNUSED_RESULT;
void WriteToPickle(const content::WebContents& web_contents,
base::Pickle* pickle);

// |web_contents| will not be modified if function returns false.
bool RestoreFromPickle(base::PickleIterator* iterator,
Expand All @@ -44,17 +44,16 @@ const uint32_t AW_STATE_VERSION_DATA_URL = 20151204;
// Functions below are individual helper functions called by functions above.
// They are broken up for unit testing, and should not be called out side of
// tests.
bool WriteHeaderToPickle(base::Pickle* pickle) WARN_UNUSED_RESULT;
bool WriteHeaderToPickle(uint32_t state_version,
base::Pickle* pickle) WARN_UNUSED_RESULT;
void WriteHeaderToPickle(base::Pickle* pickle);
void WriteHeaderToPickle(uint32_t state_version, base::Pickle* pickle);
uint32_t RestoreHeaderFromPickle(base::PickleIterator* iterator)
WARN_UNUSED_RESULT;
bool IsSupportedVersion(uint32_t state_version) WARN_UNUSED_RESULT;
bool WriteNavigationEntryToPickle(const content::NavigationEntry& entry,
base::Pickle* pickle) WARN_UNUSED_RESULT;
bool WriteNavigationEntryToPickle(uint32_t state_version,
void WriteNavigationEntryToPickle(const content::NavigationEntry& entry,
base::Pickle* pickle);
void WriteNavigationEntryToPickle(uint32_t state_version,
const content::NavigationEntry& entry,
base::Pickle* pickle) WARN_UNUSED_RESULT;
base::Pickle* pickle);
bool RestoreNavigationEntryFromPickle(base::PickleIterator* iterator,
content::NavigationEntry* entry)
WARN_UNUSED_RESULT;
Expand Down
35 changes: 15 additions & 20 deletions android_webview/browser/state_serializer_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ std::unique_ptr<content::NavigationEntry> CreateNavigationEntry() {

TEST(AndroidWebViewStateSerializerTest, TestHeaderSerialization) {
base::Pickle pickle;
bool result = internal::WriteHeaderToPickle(&pickle);
EXPECT_TRUE(result);
internal::WriteHeaderToPickle(&pickle);

base::PickleIterator iterator(pickle);
uint32_t version = internal::RestoreHeaderFromPickle(&iterator);
Expand All @@ -76,9 +75,7 @@ TEST(AndroidWebViewStateSerializerTest, TestHeaderSerialization) {

TEST(AndroidWebViewStateSerializerTest, TestLegacyVersionHeaderSerialization) {
base::Pickle pickle;
bool result = internal::WriteHeaderToPickle(
internal::AW_STATE_VERSION_INITIAL, &pickle);
EXPECT_TRUE(result);
internal::WriteHeaderToPickle(internal::AW_STATE_VERSION_INITIAL, &pickle);

base::PickleIterator iterator(pickle);
uint32_t version = internal::RestoreHeaderFromPickle(&iterator);
Expand All @@ -88,8 +85,7 @@ TEST(AndroidWebViewStateSerializerTest, TestLegacyVersionHeaderSerialization) {
TEST(AndroidWebViewStateSerializerTest,
TestUnsupportedVersionHeaderSerialization) {
base::Pickle pickle;
bool result = internal::WriteHeaderToPickle(20000101, &pickle);
EXPECT_TRUE(result);
internal::WriteHeaderToPickle(20000101, &pickle);

base::PickleIterator iterator(pickle);
uint32_t version = internal::RestoreHeaderFromPickle(&iterator);
Expand All @@ -106,13 +102,13 @@ TEST(AndroidWebViewStateSerializerTest, TestNavigationEntrySerialization) {
std::unique_ptr<content::NavigationEntry> entry(CreateNavigationEntry());

base::Pickle pickle;
bool result = internal::WriteNavigationEntryToPickle(*entry, &pickle);
EXPECT_TRUE(result);
internal::WriteNavigationEntryToPickle(*entry, &pickle);

std::unique_ptr<content::NavigationEntry> copy(
content::NavigationEntry::Create());
base::PickleIterator iterator(pickle);
result = internal::RestoreNavigationEntryFromPickle(&iterator, copy.get());
bool result =
internal::RestoreNavigationEntryFromPickle(&iterator, copy.get());
EXPECT_TRUE(result);

EXPECT_EQ(entry->GetURL(), copy->GetURL());
Expand Down Expand Up @@ -143,14 +139,13 @@ TEST(AndroidWebViewStateSerializerTest,
std::unique_ptr<content::NavigationEntry> entry(CreateNavigationEntry());

base::Pickle pickle;
bool result = internal::WriteNavigationEntryToPickle(
internal::AW_STATE_VERSION_INITIAL, *entry, &pickle);
EXPECT_TRUE(result);
internal::WriteNavigationEntryToPickle(internal::AW_STATE_VERSION_INITIAL,
*entry, &pickle);

std::unique_ptr<content::NavigationEntry> copy(
content::NavigationEntry::Create());
base::PickleIterator iterator(pickle);
result = internal::RestoreNavigationEntryFromPickle(
bool result = internal::RestoreNavigationEntryFromPickle(
internal::AW_STATE_VERSION_INITIAL, &iterator, copy.get());
EXPECT_TRUE(result);

Expand Down Expand Up @@ -183,13 +178,13 @@ TEST(AndroidWebViewStateSerializerTest, TestEmptyDataURLSerialization) {
EXPECT_FALSE(entry->GetDataURLAsString());

base::Pickle pickle;
bool result = internal::WriteNavigationEntryToPickle(*entry, &pickle);
EXPECT_TRUE(result);
internal::WriteNavigationEntryToPickle(*entry, &pickle);

std::unique_ptr<content::NavigationEntry> copy(
content::NavigationEntry::Create());
base::PickleIterator iterator(pickle);
result = internal::RestoreNavigationEntryFromPickle(&iterator, copy.get());
bool result =
internal::RestoreNavigationEntryFromPickle(&iterator, copy.get());
EXPECT_TRUE(result);
EXPECT_FALSE(entry->GetDataURLAsString());
}
Expand All @@ -212,13 +207,13 @@ TEST(AndroidWebViewStateSerializerTest, TestHugeDataURLSerialization) {
}

base::Pickle pickle;
bool result = internal::WriteNavigationEntryToPickle(*entry, &pickle);
EXPECT_TRUE(result);
internal::WriteNavigationEntryToPickle(*entry, &pickle);

std::unique_ptr<content::NavigationEntry> copy(
content::NavigationEntry::Create());
base::PickleIterator iterator(pickle);
result = internal::RestoreNavigationEntryFromPickle(&iterator, copy.get());
bool result =
internal::RestoreNavigationEntryFromPickle(&iterator, copy.get());
EXPECT_TRUE(result);
EXPECT_EQ(huge_data_url, copy->GetDataURLAsString()->data());
}
Expand Down
8 changes: 2 additions & 6 deletions base/debug/activity_tracker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1133,12 +1133,8 @@ GlobalActivityTracker::ModuleInfoRecord::CreateFrom(
const GlobalActivityTracker::ModuleInfo& info,
PersistentMemoryAllocator* allocator) {
Pickle pickler;
bool okay =
pickler.WriteString(info.file) && pickler.WriteString(info.debug_file);
if (!okay) {
NOTREACHED();
return nullptr;
}
pickler.WriteString(info.file);
pickler.WriteString(info.debug_file);
size_t required_size = offsetof(ModuleInfoRecord, pickle) + pickler.size();
ModuleInfoRecord* record = allocator->New<ModuleInfoRecord>(required_size);
if (!record)
Expand Down
28 changes: 8 additions & 20 deletions base/metrics/field_trial.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,37 +65,28 @@ const char kAllocatorName[] = "FieldTrialAllocator";
const size_t kFieldTrialAllocationSize = 128 << 10; // 128 KiB

// Writes out string1 and then string2 to pickle.
bool WriteStringPair(Pickle* pickle,
void WriteStringPair(Pickle* pickle,
const StringPiece& string1,
const StringPiece& string2) {
if (!pickle->WriteString(string1))
return false;
if (!pickle->WriteString(string2))
return false;
return true;
pickle->WriteString(string1);
pickle->WriteString(string2);
}

// Writes out the field trial's contents (via trial_state) to the pickle. The
// format of the pickle looks like:
// TrialName, GroupName, ParamKey1, ParamValue1, ParamKey2, ParamValue2, ...
// If there are no parameters, then it just ends at GroupName.
bool PickleFieldTrial(const FieldTrial::State& trial_state, Pickle* pickle) {
if (!WriteStringPair(pickle, *trial_state.trial_name,
*trial_state.group_name)) {
return false;
}
void PickleFieldTrial(const FieldTrial::State& trial_state, Pickle* pickle) {
WriteStringPair(pickle, *trial_state.trial_name, *trial_state.group_name);

// Get field trial params.
std::map<std::string, std::string> params;
FieldTrialParamAssociator::GetInstance()->GetFieldTrialParamsWithoutFallback(
*trial_state.trial_name, *trial_state.group_name, &params);

// Write params to pickle.
for (const auto& param : params) {
if (!WriteStringPair(pickle, param.first, param.second))
return false;
}
return true;
for (const auto& param : params)
WriteStringPair(pickle, param.first, param.second);
}

// Created a time value based on |year|, |month| and |day_of_month| parameters.
Expand Down Expand Up @@ -1358,10 +1349,7 @@ void FieldTrialList::AddToAllocatorWhileLocked(
return;

Pickle pickle;
if (!PickleFieldTrial(trial_state, &pickle)) {
NOTREACHED();
return;
}
PickleFieldTrial(trial_state, &pickle);

size_t total_size = sizeof(FieldTrial::FieldTrialEntry) + pickle.size();
FieldTrial::FieldTrialRef ref = allocator->Allocate(
Expand Down
Loading

0 comments on commit 0d89f92

Please sign in to comment.