1284204 - Parse complete Safe Browsing V4 updates into a new TableUpdate class

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dimi Lee [:dimi]]

In V2, we use TableUpdate structure to store prefix , chunknum ... etc information and use it to update hashstore and lookup cache.

In V4, we will also need a similar structure to do it.

Comment 1

[Dimi Lee [:dimi]]

Attachment: (WIP) TableUpdate v4

Hi Henry,
Please see if the interface provide by tableUpdateV4 in this patch fit your design of ProtocolParser and feel free to give any suggestion about this patch, thanks!

Comment 2

[Henry Chang [:hchang]]

Comment on attachment 8773561 [details] [diff] [review]
(WIP) TableUpdate v4

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

If we are going to take the approach that duplicates a copy from protobuf objects to our own, the interface definitely looks good to me except for

1) The ctor of TableUpdateV4 might take just a protobuf object
2) Since TableUpdateV4 is supposed to be immutable, all the setters in TableUpdateV4 may be not necessary. (TBD)

If we want to save the memory copy cost, we can still preserve the current interfaces but have them return objects which we can move or share memory with protobuf.

Comment 3

[Dimi Lee [:dimi]]

Attachment: (WIP) TableUpdate v4 - without copying rawhashes

Comment 4

[Henry Chang [:hchang]]

Comment on attachment 8774757 [details] [diff] [review]
(WIP) TableUpdate v4 - without copying rawhashes

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

::: toolkit/components/url-classifier/HashStore.h
@@ +131,5 @@
> +
> +  virtual int Tag() const override { return TAG; }
> +};
> +
> +struct PrefixString {

I wonder if we can just merge its member variables to TableUpdateV4. At least, we can put this struct definition inside TableUpdateV4

@@ +134,5 @@
> +
> +struct PrefixString {
> +private:
> +  std::string mStorage;
> +  nsDependentCString mString;

Should be nsDependentCSubstring

@@ +138,5 @@
> +  nsDependentCString mString;
> +
> +public:
> +  PrefixString(std::string& aString) 
> +    : mString(nullptr)

This initializer is wrong and not needed

Comment 5

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1284204-Support-new-TableUpdate-structure-for-Sa.patch

Comment 6

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1284204-Support-new-TableUpdate-structure-for-Sa.patch

Comment 7

[Henry Chang [:hchang]]

Took this bug from Dimi since we have come up with a interfaces to work with. I will continue working on the patch and add changes to ProtocolParserProtobuf.

Comment 9

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1284204-Support-new-TableUpdate-structure-for-Sa.patch

Comment 10

[Dimi Lee [:dimi]]

Comment on attachment 8777690 [details] [diff] [review]
0001-Bug-1284204-Support-new-TableUpdate-structure-for-Sa.patch

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

Looks really good, thank you!

::: toolkit/components/url-classifier/HashStore.cpp
@@ +505,5 @@
>  {
> +  auto updateV2 = TableUpdate::Cast<TableUpdateV2>(&aUpdate);
> +  if (!updateV2) {
> +    return NS_ERROR_FAILURE;
> +  }

NS_ENSURE_TRUE(updateV2, NS_ERROR_FAILURE);

::: toolkit/components/url-classifier/ProtocolParser.cpp
@@ +796,5 @@
> +  auto tu = GetTableUpdate(nsCString(listName.get()));
> +  auto tuV4 = TableUpdate::Cast<TableUpdateV4>(tu);
> +  if (!tuV4) {
> +    return NS_ERROR_FAILURE;
> +  }

ditto.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +711,5 @@
> +
> +      TableUpdateV2* tuV2 = TableUpdate::Cast<TableUpdateV2>(tu);
> +      if (!tuV2) {
> +        return NS_ERROR_FAILURE;
> +      }

ditto.

@@ +717,3 @@
>        LOG(("CacheCompletion Addchunk %d hash %X", resultsPtr->ElementAt(i).entry.addChunk,
>             resultsPtr->ElementAt(i).entry.ToUint32()));
> +      rv = tuV2->NewAddComplete(resultsPtr->ElementAt(i).entry.addChunk,

Nits. Indent

Comment 11

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1284204-Support-new-TableUpdate-structure-for-Sa.patch

Comment 12

[Henry Chang [:hchang]]

Attachment: Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

Review commit: https://reviewboard.mozilla.org/r/70276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70276/

Comment 13

[François Marier [:francois]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

https://reviewboard.mozilla.org/r/70276/#review68134

Your approach looks good to me.

I've got two questions for you:

1. Have you thought about making `ProtocolParser` an abstract base class like you did with `TableUpdate`? We could have `ProtocolParserProtobuf`, `ProtocolParserShavar` and `ProtocolParserSimple`.
2. From a feature point of view, what does landing this patch do? Does it stand on its own or should we land it at the same time as the storage patch?

::: toolkit/components/url-classifier/HashStore.h:28
(Diff revision 1)
> +// The abstract class of TableUpdateV2 and TableUpdateV4. This
> +// is convenient for passing the TableUpdate* around associated
> +// with v2 and v4 instance.
> +class TableUpdate {
> +public:
> +  TableUpdate(const nsACString& aTable)

Maybe this should be `explicit` too?

::: toolkit/components/url-classifier/HashStore.h:38
(Diff revision 1)
> +
> +  virtual ~TableUpdate() {}
> +
> +  // To be overriden.
> +  virtual bool Empty() const = 0;
> +  virtual int Tag() const = 0;

Does this need to be `public`? Can it be `protected` instead?

::: toolkit/components/url-classifier/HashStore.h:65
(Diff revision 1)
>  public:
> -  explicit TableUpdate(const nsACString& aTable)
> -      : mTable(aTable), mLocalUpdate(false) {}
> +  explicit TableUpdateV2(const nsACString& aTable)
> +    : TableUpdate(aTable) {}
> -  const nsCString& TableName() const { return mTable; }
>  
>    bool Empty() const {

nit: You could add the `override` keyword here just like you did in `Tag()`.

::: toolkit/components/url-classifier/HashStore.h:114
(Diff revision 1)
>    SubPrefixArray& SubPrefixes() { return mSubPrefixes; }
>    AddCompleteArray& AddCompletes() { return mAddCompletes; }
>    SubCompleteArray& SubCompletes() { return mSubCompletes; }
>  
> +  // For downcasting.
> +  static const int TAG = 2;

nit: Maybe we could use an enum here?

Something like:

    enum class PVer { V2_2, V4 };

I'm thinking it would restrict the value of member to things that work, but also, it would allow us to create a separate class for the  4.5 if that every becomes necessary (which an int wouldn't allow us to do).

::: toolkit/components/url-classifier/HashStore.h:137
(Diff revision 1)
> +  virtual int Tag() const override { return TAG; }
> +};
> +
> +// For v4 table update.
> +//
> +// TODO: Interfaces for DBService/HashStore/Classifiers to udpate.

typo: udpate

::: toolkit/components/url-classifier/HashStore.h:159
(Diff revision 1)
> +
> +  typedef nsClassHashtable<nsUint32HashKey, PrefixString> PrefixesStringMap;
> +  typedef nsTArray<int32_t> RemovalIndiceArray;
> +
> +public:
> +  TableUpdateV4(const nsACString& aTable)

Maybe this should be `explicit` too?

::: toolkit/components/url-classifier/HashStore.h:164
(Diff revision 1)
> +  TableUpdateV4(const nsACString& aTable)
> +    : TableUpdate(aTable)
> +  {
> +  }
> +
> +  bool Empty() const

Again, `override`.

::: toolkit/components/url-classifier/HashStore.h:166
(Diff revision 1)
> +  {
> +  }
> +
> +  bool Empty() const
> +  {
> +    return mPrefixesMap.Count() == 0 && mRemovalIndiceArray.Length() == 0;

nit: There's probably no perf differences, but you could use `mPrefixesMap.IsEmpty() && mRemovalIndiceArray.IsEmpty()`.

::: toolkit/components/url-classifier/HashStore.cpp:167
(Diff revision 1)
>  }
>  
> +void
> +TableUpdateV4::NewPrefixes(int32_t aSize, std::string& aPrefixes)
> +{
> +  MOZ_ASSERT(aPrefixes.size() % aSize == 0);

Maybe this should be an `NS_ENSURE` instead since it could happen for real if the server returned garbage to us, right?

::: toolkit/components/url-classifier/ProtocolParser.h:140
(Diff revision 1)
>    virtual void End() override;
>  
>  private:
>    virtual ~ProtocolParserProtobuf();
>  
> +  virtual TableUpdate* CreateTableUpdate(const nsACString& aTableName) const;

Missing `override`

::: toolkit/components/url-classifier/ProtocolParser.cpp:86
(Diff revision 1)
>    mCryptoHash = aHasher;
>    return NS_OK;
>  }
>  
>  void
>  ProtocolParser::SetCurrentTable(const nsACString& aTable)

To make sure this doesn't get called in the Protobuf Protocol parser, maybe we should override this method and put an `assert(false)` in it?

::: toolkit/components/url-classifier/ProtocolParser.cpp:89
(Diff revision 1)
>  
>  void
>  ProtocolParser::SetCurrentTable(const nsACString& aTable)
>  {
> -  mTableUpdate = GetTableUpdate(aTable);
> +  auto update = GetTableUpdate(aTable);
> +  mTableUpdate = TableUpdate::Cast<TableUpdateV2>(update);

Maybe we should check that the cast didn't return `nullptr`?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:710
(Diff revision 1)
>          activeTable = true;
>          break;
>        }
>      }
>      if (activeTable) {
>        TableUpdate * tu = pParse->GetTableUpdate(resultsPtr->ElementAt(i).table);

nit: I would personally use this trick to ensure that we never accidentally use the `tu` variable directly:

```
TableUpdateV2* tuV2 = nullptr;
{
  TableUpdate * tu = pParse->GetTableUpdate(resultsPtr->ElementAt(i).table);

  tuV2 = TableUpdate::Cast<TableUpdateV2>(tu);
}
NS_ENSURE_TRUE(tuV2, NS_ERROR_FAILURE);
```

i.e. the `tu` variable is innaccessible as soon as it's no longer useful.

Comment 14

[Henry Chang [:hchang]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

https://reviewboard.mozilla.org/r/70276/#review68134

1. I have but there's no strong reason encouraging me. (The only reason to me is that would make it look awesome :)) However, as for TableUpdate, TableUpdateV2 and TableUpdateV4 has almost different interfaces. It we just make TableUpdateV4 inherit the original TableUpdate, TableUpdateV4 would inherit lots of unsupported interfaces which needs to be made private (or TableUpdateV4 should just "privately" inherit TableUpdate.)

2. It does nothing from a feature point of view :( This patch could be merged to Bug 1285848 (supporting RICE-encoded update) OR Bug 1283009 (storing prefix to HashStore) and that's exactly the reason I'd like to land this patch standalone. Otherwise, we have to merge Bug 1285848 and Bug 1283009 since they both depend on this patch. 

BTW, by your definition, do you think Bug 1285848 (supporting RICE-encoding update) also does nothing from a feature point of view? It actually does nothing beyond this patch and only decode then put update to TableUpdateV4.

> nit: Maybe we could use an enum here?
> 
> Something like:
> 
>     enum class PVer { V2_2, V4 };
> 
> I'm thinking it would restrict the value of member to things that work, but also, it would allow us to create a separate class for the  4.5 if that every becomes necessary (which an int wouldn't allow us to do).

I'd consider |TAG| a distinct ID to for each derived class for implementing a lightweight RTTI. It doesn't matter what the value it is and the only restriction is it has to be distinct. IMHO, using a enum doesn't help guarantee the distinctness. (or does it?)

> Maybe this should be `explicit` too?

Definitely since there's no implicit casting requirement around this class :)

> nit: There's probably no perf differences, but you could use `mPrefixesMap.IsEmpty() && mRemovalIndiceArray.IsEmpty()`.

Good point! Thanks!

> To make sure this doesn't get called in the Protobuf Protocol parser, maybe we should override this method and put an `assert(false)` in it?

Good idea :)

> Maybe we should check that the cast didn't return `nullptr`?

I would use an ASSERT since it should never occur :)

> nit: I would personally use this trick to ensure that we never accidentally use the `tu` variable directly:
> 
> ```
> TableUpdateV2* tuV2 = nullptr;
> {
>   TableUpdate * tu = pParse->GetTableUpdate(resultsPtr->ElementAt(i).table);
> 
>   tuV2 = TableUpdate::Cast<TableUpdateV2>(tu);
> }
> NS_ENSURE_TRUE(tuV2, NS_ERROR_FAILURE);
> ```
> 
> i.e. the `tu` variable is innaccessible as soon as it's no longer useful.

Good idea! Thanks!

Comment 15

[François Marier [:francois]]

(In reply to Henry Chang [:henry][:hchang] from comment #14)
> 1. I have but there's no strong reason encouraging me. (The only reason to
> me is that would make it look awesome :))

The main reason that came to my mind was that if the protocol parsers were separate like that, we could more easily remove the V2 code or the -simple lists if we wanted. Of course, we're not going to do that anytime soon, so we could always worry about that later.

I suppose it also feels a little strange that the ProtocolParserProtobuf class would technically have access to this code to parse V2 things. If it inherited from an base class, it wouldn't have any way to call the parent class' functions.

> 2. It does nothing from a feature point of view :( This patch could be
> merged to Bug 1285848 (supporting RICE-encoded update) OR Bug 1283009
> (storing prefix to HashStore) and that's exactly the reason I'd like to land
> this patch standalone. Otherwise, we have to merge Bug 1285848 and Bug
> 1283009 since they both depend on this patch.

Is it fair to say that this patch would fully parse and validate the (non-Rice-encoded) updates that we get from Google and then store them carefully in /dev/null? That sounds like a useful step towards Milestone 1 and something that can be tested.

Anyways, I guess what I was trying to get to is that the bug title (and therefore the commit message) should describe what this does as opposed to how it's implemented :)

> I'd consider |TAG| a distinct ID to for each derived class for implementing
> a lightweight RTTI. It doesn't matter what the value it is and the only
> restriction is it has to be distinct. IMHO, using a enum doesn't help
> guarantee the distinctness. (or does it?)

Right. It wouldn't enforce distinctness, but it would ensure that the value is always valid since it comes from a restricted set.

Anyways, that's a nit, up to you :)

> > Maybe we should check that the cast didn't return `nullptr`?
> 
> I would use an ASSERT since it should never occur :)

Famous last words ;-)

Comment 16

[Henry Chang [:hchang]]

(In reply to François Marier [:francois] from comment #15)
> (In reply to Henry Chang [:henry][:hchang] from comment #14)
> > 1. I have but there's no strong reason encouraging me. (The only reason to
> > me is that would make it look awesome :))
> 
> The main reason that came to my mind was that if the protocol parsers were
> separate like that, we could more easily remove the V2 code or the -simple
> lists if we wanted. Of course, we're not going to do that anytime soon, so
> we could always worry about that later.
> 

This is a pretty good reason to me actually!

> I suppose it also feels a little strange that the ProtocolParserProtobuf
> class would technically have access to this code to parse V2 things. If it
> inherited from an base class, it wouldn't have any way to call the parent
> class' functions.
> 

It wouldn't because all V2 parsing things are made private.

I am positive to refactor around ProtocolParser and will do it this time
since making removal of V2 code more easily is a very appealing reason :)

> > 2. It does nothing from a feature point of view :( This patch could be
> > merged to Bug 1285848 (supporting RICE-encoded update) OR Bug 1283009
> > (storing prefix to HashStore) and that's exactly the reason I'd like to land
> > this patch standalone. Otherwise, we have to merge Bug 1285848 and Bug
> > 1283009 since they both depend on this patch.
> 
> Is it fair to say that this patch would fully parse and validate the
> (non-Rice-encoded) updates that we get from Google and then store them
> carefully in /dev/null? That sounds like a useful step towards Milestone 1
> and something that can be tested.
> 
> Anyways, I guess what I was trying to get to is that the bug title (and
> therefore the commit message) should describe what this does as opposed to
> how it's implemented :)
> 

So would you think we can land this patch at this time after getting r+?

Thanks for your reply!

Comment 17

[François Marier [:francois]]

(In reply to Henry Chang [:henry][:hchang] from comment #16)
> > Anyways, I guess what I was trying to get to is that the bug title (and
> > therefore the commit message) should describe what this does as opposed to
> > how it's implemented :)
> 
> So would you think we can land this patch at this time after getting r+?

Yes, I think it makes sense to do that as long as we give that patch/commit a better description of why it's useful :)

Comment 18

[Henry Chang [:hchang]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70276/diff/1-2/

Comment 19

[François Marier [:francois]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

https://reviewboard.mozilla.org/r/70276/#review68874

I like the ProtocolParser base class, it seems like we might be able to simplify a few things. See comments below.

::: toolkit/components/url-classifier/HashStore.h:44
(Diff revisions 1 - 2)
>    // Common interfaces.
>    void SetLocalUpdate(void) { mLocalUpdate = true; }
>    bool IsLocalUpdate(void) { return mLocalUpdate; }
>    const nsCString& TableName() const { return mTable; }
>  
>    template<typename T>

I think this Cast function might be unnecesary and could be replaced by a `dynamic_cast`, no?

::: toolkit/components/url-classifier/HashStore.h:50
(Diff revisions 1 - 2)
>    static T* Cast(TableUpdate* aThat) {
>      return (T::TAG == aThat->Tag() ? reinterpret_cast<T*>(aThat) : nullptr);
>    }
>  
>  private:
> +  virtual int Tag() const = 0;

If we get rid of the `Cast()` method, then we don't need these tags.

::: toolkit/components/url-classifier/HashStore.h:138
(Diff revisions 1 - 2)
>    virtual int Tag() const override { return TAG; }
>  };
>  
>  // For v4 table update.
>  //
> -// TODO: Interfaces for DBService/HashStore/Classifiers to udpate.
> +// TODO: Interfaces for DBService/HashStore/Classifiers to update.

nit: might be good to add the bugnumber where Dimi is doing that work

::: toolkit/components/url-classifier/ProtocolParser.h:17
(Diff revisions 1 - 2)
>  
>  namespace mozilla {
>  namespace safebrowsing {
>  
>  /**
> - * Helpers to parse the "shavar", "digest256" and "simple" list formats.
> + * Abstract base class for parsing update data in variant formats.

grammar: I would use "multiple" instead of "variant"

::: toolkit/components/url-classifier/ProtocolParser.h:48
(Diff revisions 1 - 2)
>    // becomes the caller's responsibility to free them.  This is ****.
>    TableUpdate *GetTableUpdate(const nsACString& aTable);
>    void ForgetTableUpdates() { mTableUpdates.Clear(); }
>    nsTArray<TableUpdate*> &GetTableUpdates() { return mTableUpdates; }
>  
> +  // Update information. These are only meaningful for V2.

Can't these just live in the V2 class?

::: toolkit/components/url-classifier/ProtocolParser.cpp:130
(Diff revisions 1 - 2)
> +
> +void
> +ProtocolParserV2::SetCurrentTable(const nsACString& aTable)
>  {
>    auto update = GetTableUpdate(aTable);
>    mTableUpdate = TableUpdate::Cast<TableUpdateV2>(update);

I don't think we need `Cast()` anymore because `GetTableUpdate()` will always return a `TableUpdateV2` when it's called on a `ProtocolParserV2` object.

::: toolkit/components/url-classifier/ProtocolParser.cpp:820
(Diff revisions 1 - 2)
>      NS_WARNING("New state not initialized.");
>      return NS_ERROR_FAILURE;
>    }
>  
>    auto tu = GetTableUpdate(nsCString(listName.get()));
>    auto tuV4 = TableUpdate::Cast<TableUpdateV4>(tu);

Could be a `dynamic_cast`?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:464
(Diff revisions 1 - 2)
>                   "within the same provider.");
>        break;
>      }
>    }
>  
> -  mProtocolParser = (useProtobuf ? new ProtocolParserProtobuf()
> +  mProtocolParser = (useProtobuf ? static_cast<ProtocolParser*>(new ProtocolParserProtobuf())

Are these casts necessary?

`ProtocolParserProtobuf*` objects are also `ProtocolParser*` objects...

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:710
(Diff revisions 1 - 2)
>          activeTable = true;
>          break;
>        }
>      }
>      if (activeTable) {
> -      TableUpdate * tu = pParse->GetTableUpdate(resultsPtr->ElementAt(i).table);
> +      TableUpdateV2* tuV2 = TableUpdate::Cast<TableUpdateV2>(

Can't we just use a `dynamic_cast` here?

Comment 20

[Henry Chang [:hchang]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

https://reviewboard.mozilla.org/r/70276/#review68874

Thanks for your review again :) The dynamic_cast is not allowed so the only issue I am going to fix is to add bug number in the next patch. Thanks!

> I think this Cast function might be unnecesary and could be replaced by a `dynamic_cast`, no?

RTTI is disabled in gecko except for some third party libraries as far as I know. That's why I had to implement the lightweight RTTI things :)

> Can't these just live in the V2 class?

I would feel |AppendStream| and |End| is generic enough to be a base interfaces. |SetCurrentTable| doesn't make much sense to V4 but at least it's not V2 specific.

> I don't think we need `Cast()` anymore because `GetTableUpdate()` will always return a `TableUpdateV2` when it's called on a `ProtocolParserV2` object.

Makes sense!

> Are these casts necessary?
> 
> `ProtocolParserProtobuf*` objects are also `ProtocolParser*` objects...

It's necessary or the compiler would complain ^^"

http://stackoverflow.com/questions/3833736/why-would-i-need-conversion

Comment 21

[Henry Chang [:hchang]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

https://reviewboard.mozilla.org/r/70276/#review68874

> Makes sense!

Oops! It still has to be cast since the return type of GetTableUpdate() is |TableUpdate*| but not TableUpdateV2.

Comment 22

[Henry Chang [:hchang]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70276/diff/2-3/

Comment 23

[François Marier [:francois]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

(In reply to Henry Chang [:henry][:hchang] from comment #20)
> Thanks for your review again :) The dynamic_cast is not allowed so the only
> issue I am going to fix is to add bug number in the next patch. Thanks!

Could you also update the commit message to match the new bug title? It will hopefully be a bit more descriptive.

> > I think this Cast function might be unnecesary and could be replaced by a `dynamic_cast`, no?
> 
> RTTI is disabled in gecko except for some third party libraries as far as I
> know. That's why I had to implement the lightweight RTTI things :)

Ah, I see. That makes sense.

> > Can't these just live in the V2 class?
> 
> I would feel |AppendStream| and |End| is generic enough to be a base
> interfaces. |SetCurrentTable| doesn't make much sense to V4 but at least
> it's not V2 specific.

Sorry, the line the comment was attached to was a little confusing. I meant shouldn't these live in the V2 class:

    virtual const nsTArray<ForwardedUpdate> &Forwards() const { return mForwards; }
    virtual int32_t UpdateWait() { return 0; }
    virtual bool ResetRequested() { return false; }

According to the comment you added, they're only used in V2.

> > Are these casts necessary?
> > 
> > `ProtocolParserProtobuf*` objects are also `ProtocolParser*` objects...
> 
> It's necessary or the compiler would complain ^^"
> 
> http://stackoverflow.com/questions/3833736/why-would-i-need-conversion

Thanks for the link, that was an interesting discussion!

Comment 24

[Henry Chang [:hchang]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70276/diff/3-4/

Comment 25

[François Marier [:francois]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

https://reviewboard.mozilla.org/r/70276/#review69542

Looks good. There's a tiny typo you might want to fix before pushing.

Aside from that, I would suggest updating the commit message to match the new bug title. r+

::: toolkit/components/url-classifier/HashStore.h:137
(Diff revisions 3 - 4)
>  
>    virtual int Tag() const override { return TAG; }
>  };
>  
> -// For v4 table update.
> -//
> +// Structure for DBService/HashStore/Classifiers to update.
> +// It would contain the preifxes (both fixed and variable length)

typo: preifxes

Comment 26

[Henry Chang [:hchang]]

Comment on attachment 8779229 [details]
Bug 1284204 - Parse SafeBrowsing V4 updates to TableUpdateV4. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70276/diff/4-5/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/976503649744
Parse SafeBrowsing V4 updates to TableUpdateV4. r=francois.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/976503649744