Closed Bug 1385461 Opened 7 years ago Closed 7 years ago

Update our bundled copy of the protocol buffers library

Categories

(Core :: General, enhancement, P1)

Product:
Core
Component:
General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: francois, Assigned: francois)

References

(Blocks 1 open bug)

Details

(Whiteboard: pwphish-prep)

Attachments

(8 files, 1 obsolete file)

Bug 1385461 - Re-sync LayerScopePacket.proto with the generated files.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1385461 - Move URL Classifier protobuf files to match other components.
59 bytes, text/x-review-board-request
dlee
: review+
Details
Bug 1385461 - Fix upgrade script to avoid excluding bytestream files.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1385461 - Add script to regenerate all .pb.cc and .pb.h files.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1385461 - Upgrade to the latest version of the protobuf library.
59 bytes, text/x-review-board-request
dlee
: review+
mossop
: review+
mattwoodrow
: review+
fitzgen
: review+
glandium
: review+
Details
Bug 1385461 - Document the reason for the time.h-including cc files in SOURCES.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1385461 - Document the reason for the json_escaping.h-including cc files in SOURCES.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1385461 - Document the reason for wire_format.cc in SOURCES.
59 bytes, text/x-review-board-request
glandium
: review+
Details 
We are using an old version of the protobuf library (2.6.1) and this has now diverged enough from the one that Chrome uses that we can no longer use their protobuf definitions directly (e.g. bug 1384753).

Updating to the latest version (currently 3.3.0) will involve re-generating all of the .pb.cc and .pb.h files that are in our codebase:

devtools/shared/heapsnapshot/CoreDump.pb.h
gfx/layers/protobuf/LayerScopePacket.pb.h
toolkit/components/downloads/chromium/chrome/common/safe_browsing/csd.pb.h
toolkit/components/url-classifier/protobuf/safebrowsing.pb.h
NI'd Patrick and Milan for insight into how much work it is to upgrade for them.
Flags: needinfo?(pbrosset)
Flags: needinfo?(milan)
Boris should have a good idea about the LayerScope stuff.
Flags: needinfo?(milan) → needinfo?(boris.chiou)
Yes. it's fine to update gfx/layers/protobuf/LayerScopePacket.pb.h. We easily update this file on the JS side in layerscope, based on the new protocol buffer library.
Flags: needinfo?(boris.chiou)
Bouncing this too Jim who will know more than me on this specific topic.
Flags: needinfo?(pbrosset) → needinfo?(jimb)
Has the serialized form changed? Or just the syntax of the .proto files?
Flags: needinfo?(jimb) → needinfo?(francois)
(In reply to Jim Blandy :jimb from comment #5)
> Has the serialized form changed? Or just the syntax of the .proto files?

I'm not sure about the serialized form, but the syntax has changed and I can't import the latest .proto files from chromium without patching them.
Flags: needinfo?(francois)
:francois can you create some sub bugs for this for each of the components? And lets get this work prioritized for Q4.
Flags: needinfo?(francois)
Priority: -- → P1
(In reply to Selena Deckelmann :selenamarie :selena use ni? pronoun: she from comment #7)
> :francois can you create some sub bugs for this for each of the components?
> And lets get this work prioritized for Q4.

I'm not sure sub-bugs make sense because as far as I know it all has to happen at once.

You can't regenerate the protobuf files until the library has been updated and you can't land the updated library without re-generating these protobuf files. In my limited testing of this, the compile checks seemed to go both ways.
Flags: needinfo?(francois)
(In reply to François Marier [:francois] from comment #8)
> (In reply to Selena Deckelmann :selenamarie :selena use ni? pronoun: she
> from comment #7)
> > :francois can you create some sub bugs for this for each of the components?
> > And lets get this work prioritized for Q4.
> 
> I'm not sure sub-bugs make sense because as far as I know it all has to
> happen at once.
> 
> You can't regenerate the protobuf files until the library has been updated
> and you can't land the updated library without re-generating these protobuf
> files. In my limited testing of this, the compile checks seemed to go both
> ways.

I see. Does this work need to be done by three people? It sounds like there's consensus that it can be done.
(In reply to Selena Deckelmann :selenamarie :selena use ni? pronoun: she from comment #9)
> I see. Does this work need to be done by three people? It sounds like
> there's consensus that it can be done.

My guess is one person doing it and then three other people testing to make sure their stuff still works with the re-generated protobufs.
Hi Francois, since you are familiar with this, can you pick up this.
Assignee: nobody → francois
I'll take this one
Assignee: francois → dlee
Status: NEW → ASSIGNED
(In reply to François Marier [:francois] from comment #8)
> (In reply to Selena Deckelmann :selenamarie :selena use ni? pronoun: she
> from comment #7)
> > :francois can you create some sub bugs for this for each of the components?
> > And lets get this work prioritized for Q4.
> 
> I'm not sure sub-bugs make sense because as far as I know it all has to
> happen at once.
> 
> You can't regenerate the protobuf files until the library has been updated
> and you can't land the updated library without re-generating these protobuf
> files. In my limited testing of this, the compile checks seemed to go both
> ways.

That's great if we could finish this bug. I just tried protobuf 3.3.0 to compile our safebrowsing.proto and I could successfully generate a lot of js files (which are really useful to test Safebrowsing protobuf unpacking in web content).
Assignee: dlee → francois
Status: ASSIGNED → NEW
Assignee: francois → dlee
Status: NEW → ASSIGNED
Blocks: 1403796
(In reply to Dimi Lee[:dimi][:dlee] from comment #12)
> I'll take this one

I've actually started this one (https://hg.mozilla.org/users/fmarier_mozilla.com/mozilla-unified/rev/upgrade-protobuf-1385461), but haven't yet fixed all of the build errors.
Assignee: dlee → francois
Whiteboard: pwphish-prep
No longer blocks: 1407879
Dave, this toolkit component doesn't have a specific owner so I'm not sure if you want to review this as the module owner for Toolkit, but feel free to delegate to someone else.

Dimi, can you please review the URL Classifier and Download Protection parts?

Matt, I added you for the gfx/layers bits.

Nick, could you please take a look at the devtools bits?

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acc1bf03780812545426b338458ac5cd5b29ada8
Attachment #8918495 - Flags: review?(dtownsend)
Attachment #8918496 - Flags: review?(dtownsend)
Attachment #8918498 - Flags: review?(mh+mozilla)
Attachment #8918499 - Flags: review?(mh+mozilla)
Attachment #8918500 - Flags: review?(mh+mozilla)
Mike, if you could review the build-related changes, that would be great.

BTW, I tried to set the reviewer to core-build-config-reviews@mozilla.bugs but MozReview won't accept it.
For the record, I'm trying to see if our remaining customizations can be upstreamed:

https://github.com/google/protobuf/pull/3745
https://github.com/google/protobuf/pull/3746
https://github.com/google/protobuf/pull/3747
Comment on attachment 8918493 [details]
Bug 1385461 - Re-sync LayerScopePacket.proto with the generated files.

https://reviewboard.mozilla.org/r/189358/#review194656
Attachment #8918493 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8918497 [details]
Bug 1385461 - Upgrade to the latest version of the protobuf library.

https://reviewboard.mozilla.org/r/189366/#review194658
Attachment #8918497 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8918496 [details]
Bug 1385461 - Add script to regenerate all .pb.cc and .pb.h files.

https://reviewboard.mozilla.org/r/189364/#review195126
Attachment #8918496 - Flags: review?(dtownsend) → review+
Comment on attachment 8918495 [details]
Bug 1385461 - Fix upgrade script to avoid excluding bytestream files.

https://reviewboard.mozilla.org/r/189362/#review195128
Attachment #8918495 - Flags: review?(dtownsend) → review+
Comment on attachment 8918497 [details]
Bug 1385461 - Upgrade to the latest version of the protobuf library.

https://reviewboard.mozilla.org/r/189366/#review195130
Attachment #8918497 - Flags: review?(dtownsend) → review+
Comment on attachment 8918494 [details]
Bug 1385461 - Move URL Classifier protobuf files to match other components.

https://reviewboard.mozilla.org/r/189360/#review195174
Attachment #8918494 - Flags: review?(dlee) → review+
Comment on attachment 8918497 [details]
Bug 1385461 - Upgrade to the latest version of the protobuf library.

https://reviewboard.mozilla.org/r/189366/#review195188
Attachment #8918497 - Flags: review?(dlee) → review+
Comment on attachment 8918497 [details]
Bug 1385461 - Upgrade to the latest version of the protobuf library.

https://reviewboard.mozilla.org/r/189366/#review195340

Other than the moz.build, it's all mechanical, right?
Attachment #8918497 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8918498 [details]
Bug 1385461 - Document the reason for the time.h-including cc files in SOURCES.

https://reviewboard.mozilla.org/r/189368/#review195342

::: toolkit/components/protobuf/moz.build:229
(Diff revision 1)
>  
>  SOURCES += [
>      'src/google/protobuf/extension_set_heavy.cc',
>      'src/google/protobuf/generated_message_table_driven.cc',
>      'src/google/protobuf/generated_message_table_driven_lite.cc',
> -    'src/google/protobuf/stubs/time.cc',
> +    'src/google/protobuf/stubs/time.cc', # GetCurrentTime conflict in winbase.h

2 spaces before in-line comments. (per PEP8)
Attachment #8918498 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8918499 [details]
Bug 1385461 - Document the reason for the json_escaping.h-including cc files in SOURCES.

https://reviewboard.mozilla.org/r/189370/#review195346

::: toolkit/components/protobuf/moz.build:230
(Diff revision 1)
>  SOURCES += [
>      'src/google/protobuf/extension_set_heavy.cc',
>      'src/google/protobuf/generated_message_table_driven.cc',
>      'src/google/protobuf/generated_message_table_driven_lite.cc',
>      'src/google/protobuf/stubs/time.cc', # GetCurrentTime conflict in winbase.h
> -    'src/google/protobuf/util/internal/json_escaping.cc',
> +    'src/google/protobuf/util/internal/json_escaping.cc', # namespace problem in json_escaping.h

2 spaces
Attachment #8918499 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8918500 [details]
Bug 1385461 - Document the reason for wire_format.cc in SOURCES.

https://reviewboard.mozilla.org/r/189372/#review195348

::: toolkit/components/protobuf/moz.build:238
(Diff revision 1)
>      'src/google/protobuf/util/internal/proto_writer.cc', # GetCurrentTime conflict in winbase.h
>      'src/google/protobuf/util/internal/protostream_objectsource.cc', # GetCurrentTime conflict in winbase.h
>      'src/google/protobuf/util/internal/protostream_objectwriter.cc', # GetCurrentTime conflict in winbase.h
>      'src/google/protobuf/util/message_differencer.cc',
>      'src/google/protobuf/util/time_util.cc', # GetCurrentTime conflict in winbase.h
> -    'src/google/protobuf/wire_format.cc',
> +    'src/google/protobuf/wire_format.cc', # GetMessage conflict in windows.h

2 spaces
Attachment #8918500 - Flags: review?(mh+mozilla) → review+
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #33)
> Comment on attachment 8918497 [details]
> Bug 1385461 - Update to the latest version of the protobuf library.
> 
> https://reviewboard.mozilla.org/r/189366/#review195340
> 
> Other than the moz.build, it's all mechanical, right?

Yes, the *.pb.* files are all automatically generated by the tool.
Sorry Mike, I forgot to ask for your review on the giant patch as well (for the build changes).
Loading that giant patch took my content process to the ground. Could you separate the build changes from the protobuf changes (that would also make review easier)?
Flags: needinfo?(francois)
I rebased the patch onto the latest central and managed to remove one more of our custom patches: https://reviewboard.mozilla.org/r/189366/diff/2-3/

Try is still looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da3141f2a92ea9d7547071b236563cde8b372ee4
(In reply to Mike Hommey [:glandium] from comment #43)
> Loading that giant patch took my content process to the ground. Could you
> separate the build changes from the protobuf changes (that would also make
> review easier)?

I can't easily split up that big patch any further without potentially breaking bisecting, but I've attached a patch with only the build changes from it. It removes all of the upstream code and the generated files.
Flags: needinfo?(francois)
Attachment #8920393 - Flags: review?(mh+mozilla)
Mike, here's a link that won't crash your content process and will allow you to set the r flag in MozReview: https://reviewboard.mozilla.org/r/189366/diff/2-3/

It would be nice if you could r+ it there too so that autoland records the correct list of reviewers.
Comment on attachment 8918497 [details]
Bug 1385461 - Upgrade to the latest version of the protobuf library.

https://reviewboard.mozilla.org/r/189366/#review197444

The interdiff didn't contain everything not related to the code import. Far from it. So I pulled the changeset locally and looked at it. It would still have been easier if you had split it, and squashed after review for your bisect concerns.
Attachment #8918497 - Flags: review?(mh+mozilla) → review+
Attachment #8920393 - Attachment is obsolete: true
Attachment #8920393 - Flags: review?(mh+mozilla)
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d29dc7075531
Re-sync LayerScopePacket.proto with the generated files. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/c46d9e05ec12
Move URL Classifier protobuf files to match other components. r=dimi
https://hg.mozilla.org/integration/autoland/rev/12c2e68e674d
Fix upgrade script to avoid excluding bytestream files. r=mossop
https://hg.mozilla.org/integration/autoland/rev/8f2f523109dc
Add script to regenerate all .pb.cc and .pb.h files. r=mossop
https://hg.mozilla.org/integration/autoland/rev/aca83df2ce2e
Upgrade to the latest version of the protobuf library. r=dimi,fitzgen,glandium,mattwoodrow,mossop
https://hg.mozilla.org/integration/autoland/rev/2b5488022b9f
Document the reason for the time.h-including cc files in SOURCES. r=glandium
https://hg.mozilla.org/integration/autoland/rev/cd5b5f3af517
Document the reason for the json_escaping.h-including cc files in SOURCES. r=glandium
https://hg.mozilla.org/integration/autoland/rev/c8fea1a40065
Document the reason for wire_format.cc in SOURCES. r=glandium
Patch from comment 57 caused these regressions on Linux builds. Can we address these?

== Change summary for alert #10139 (as of October 24 2017 00:34 UTC) ==

Regressions:

 26%  compiler_metrics num_static_constructors linux64-rusttests opt      103.00 -> 130.00
 26%  compiler_metrics num_static_constructors linux32-rusttests opt      103.00 -> 130.00
 26%  compiler_metrics num_static_constructors linux32 pgo                103.00 -> 130.00
 26%  compiler_metrics num_static_constructors linux64 pgo                103.00 -> 130.00
 26%  compiler_metrics num_static_constructors linux32 opt                103.00 -> 130.00
 13%  compiler_metrics num_static_constructors linux64-rusttests debug    213.00 -> 240.00
 13%  compiler_metrics num_static_constructors linux32-rusttests debug    213.00 -> 240.00
 13%  compiler_metrics num_static_constructors linux32 debug              213.00 -> 240.00
 12%  compiler_metrics num_static_constructors linux64-noopt debug        223.00 -> 250.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10139
Flags: needinfo?(francois)
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #58)
> Patch from comment 57 caused these regressions on Linux builds. Can we
> address these?

It looks like we need to exclude the upstream code from this test.

Do you know where this particular test is configured?
Flags: needinfo?(francois) → needinfo?(igoldan)
(In reply to François Marier [:francois] from comment #59)
> (In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment
> #58)
> > Patch from comment 57 caused these regressions on Linux builds. Can we
> > address these?
> 
> It looks like we need to exclude the upstream code from this test.
> 
> Do you know where this particular test is configured?

There is no exclusion to make. That test counts the number of static constructors in libxul.so after it's built. 27 is a large addition... especially when you add defines that, supposedly, should avoid them.
(In reply to Mike Hommey [:glandium] from comment #60)
> There is no exclusion to make. That test counts the number of static
> constructors in libxul.so after it's built. 27 is a large addition...
> especially when you add defines that, supposedly, should avoid them.

Is this important enough that we want to carry a patch like Chromium's:

https://cs.chromium.org/chromium/src/third_party/protobuf/patches/0003-remove-static-initializers.patch?rcl=a884aa1feaf0eba1bf11017155cdbd85d763d5a2
https://cs.chromium.org/chromium/src/third_party/protobuf/README.chromium?l=63&rcl=a884aa1feaf0eba1bf11017155cdbd85d763d5a2

even though it apparently breaks compatibility?

The upstream issue for this doesn't appear to be actively worked on: https://github.com/google/protobuf/issues/1404
Flags: needinfo?(igoldan) → needinfo?(mh+mozilla)
Yes.
Flags: needinfo?(mh+mozilla)
Depends on: 1411450
(In reply to Mike Hommey [:glandium] from comment #62)
> Yes.

Ok, I filed bug 1411450 to address that regression.
FYI, coverity found a lot of new issues in this code changes
198 new defect(s) introduced to Firefox found with Coverity Scan.
17 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.

Mostly resource leaks and uninit members
Depends on: 1457230
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: