Update the bundle format #450

jyasskin · 2019-07-05T05:01:01Z

Preview at https://jyasskin.github.io/webpackage/update-bundle-format/draft-yasskin-wpack-bundled-exchanges.html.

Sorry for the late mail: I belatedly realized the IETF deadline is Monday, and it’d be nice to have an update to the bundle format published then. We’ll be able to publish another revision at the beginning of the IETF week.

This update includes:

An invariant fallback URL, like signed exchanges have.
A version number, so we can easily know to fall back to a redirect.
Some infrastructure to identify what kind of error broke the parse, which can feed into both Network Error Logging and When to fallback redirect? #397’s discussion of when to fall back.
The index maps URLs to a Variants value + a list of the responses for each possible Variant-Key.
A new signatures section allows authorities to vouch for particular subsets of the bundle. For cross-origin trust, which I think will get defined in the loading spec, I think the requirement that the validity-url is same-origin is going to force us to make those single-origin subsets, even when there might be a certificate that is trusted to sign multiple origins. I’ve run the basic structure by @sleevi who liked that the signed fields are all together in a byte string.

…ring.

…in kinds of errors.

irori

lgtm from my side

irori · 2019-07-08T02:27:00Z

draft-yasskin-wpack-bundled-exchanges.md

@@ -289,103 +339,130 @@ steps, taking the `stream` as input.
      Note: The `ignoredSections` enables sections that supercede other sections
      to be introduced in the future. Implementations that don't implement any
      such sections are free to omit the relevant steps.
-   1. If `sectionOffsets["name"]` exists, return an error. That is, duplicate
-      sections are forbidden.
+   1. If `sectionOffsets["name"]` exists, return a "format error". That is,


A "format error" with fallbackUrl?

Thanks, done.

irori · 2019-07-08T03:09:53Z

draft-yasskin-wpack-bundled-exchanges.md

+         `location-in-responses` in responses), return an error.
+      1. Otherwise, assert that `requests`\[`parsedUrl`] does not exist, and set
+         `requests`\[`parsedUrl`] to
+         `MakeRelativeToStream(location-in-responses)`. If that returns an


Say that location-in-responses is the second and the third element of responses?

Good idea. Done.

irori · 2019-07-08T03:55:16Z

draft-yasskin-wpack-bundled-exchanges.md

@@ -739,6 +881,14 @@ at <https://www.iana.org/assignments/media-types>.

 * Required parameters: N/A


Remove "N/A"?

Whoops, done.

kinu

lgtm % some comments.

kinu · 2019-07-08T09:24:43Z

draft-yasskin-wpack-bundled-exchanges.md

-This takes the bundle's stream and returns a map ({{INFRA}}) of metadata
-containing at least keys named:
+This takes the bundle's stream and returns either an error, an error with a
+fallback URL (where an error is a "format error" or a "version error"), or a map


fallback URL == primaryUrl, is that right? Felt it could be more clearly written out somewhere earlier than 3.3, step 21.

Indeed, I've added a statement of that in 2.2.

kinu · 2019-07-08T09:29:58Z

draft-yasskin-wpack-bundled-exchanges.md

-1. If `metadata` doesn't have entries with keys "requests" and "manifest",
-   return an error.
+1. If `metadata` doesn't have entries with keys "primaryUrl", "requests" and
+   "manifest", return a "format error" with `fallbackUrl`.


At this point we must have primaryUrl (or we can't return an error with fallbackUrl)?

Yep, I've moved this to an Assert on the previous line.

draft-yasskin-wpack-bundled-exchanges.md

This is a preparation for supporting the new bundle format (#450). - Introduces `Version` type to represent spec versions. `version.Unversioned` represents the old format before #450. - `gen-bundle` now accepts `-version` flag. Note that it doesn't generate valid bundle in the new format yet.

- Adds PrimaryURL field to Bundle struct, and adds parsing / serializing code for that - Updates loadMetadata() to reflect the spec changes of #450 - Now it returns a fallback URL if available - Updated spec ref comments

This updates BundledExchangesParser to parse the new bundle format [1]. Changes: - Updated the magic header bytes [2] - Version field is added (we use implementation-specific version string "b1\0\0", which matches gen-bundle's output [3]) - Fallback URL (== Primary URL) field is added - ParseMetadata() returns error type ("format error" or "version error") and fallback URL if available - Updated spec ref comments The structure change of the index section is not reflected yet. It will be updated in the next CL. The test bundle file (hello.wbn) is generated with gen-bundle of github.com/WICG/webpackage revision a3cef2c, which supports the new bundle format except for the new index section structure. [1] WICG/webpackage#450 [2] WICG/webpackage#454 [3] WICG/webpackage#458 Bug: 969596 Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487 Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#684200}

This reverts commit d8c863a. Reason for revert: broke build, example: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8905916412276160432/+/steps/compile__with_patch_/0/stdout Original change's description: > BundledExchangesParser: Update the bundle format [1/2] > > This updates BundledExchangesParser to parse the new bundle format [1]. > > Changes: > - Updated the magic header bytes [2] > - Version field is added (we use implementation-specific version string > "b1\0\0", which matches gen-bundle's output [3]) > - Fallback URL (== Primary URL) field is added > - ParseMetadata() returns error type ("format error" or "version error") > and fallback URL if available > - Updated spec ref comments > > The structure change of the index section is not reflected yet. It will > be updated in the next CL. > > The test bundle file (hello.wbn) is generated with gen-bundle of > github.com/WICG/webpackage revision a3cef2c, which supports the new > bundle format except for the new index section structure. > > [1] WICG/webpackage#450 > [2] WICG/webpackage#454 > [3] WICG/webpackage#458 > > Bug: 969596 > Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487 > Reviewed-by: Robert Sesek <rsesek@chromium.org> > Reviewed-by: Tsuyoshi Horo <horo@chromium.org> > Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> > Cr-Commit-Position: refs/heads/master@{#684200} TBR=horo@chromium.org,kinuko@chromium.org,toyoshim@chromium.org,ksakamoto@chromium.org,rsesek@chromium.org Change-Id: Ifce2b07c15a57a2030887cbfaad5853cbc5af992 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 969596 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1737883 Reviewed-by: Andrey Kosyakov <caseq@chromium.org> Commit-Queue: Andrey Kosyakov <caseq@chromium.org> Cr-Commit-Position: refs/heads/master@{#684207}

This adds parsing / serializing code of the index section in the new (b1) format (#450). Currently, multiple responses for a single URL is not supported. After this patch, gen-bundle generates valid bundles in b1 format.

This is a reland of d8c863a PS1 is the original CL, PS2 is the fix for the new callsite of BundleMetadataParseError::New() introduced by https://crrev.com/c/1732518. Original change's description: > BundledExchangesParser: Update the bundle format [1/2] > > This updates BundledExchangesParser to parse the new bundle format [1]. > > Changes: > - Updated the magic header bytes [2] > - Version field is added (we use implementation-specific version string > "b1\0\0", which matches gen-bundle's output [3]) > - Fallback URL (== Primary URL) field is added > - ParseMetadata() returns error type ("format error" or "version error") > and fallback URL if available > - Updated spec ref comments > > The structure change of the index section is not reflected yet. It will > be updated in the next CL. > > The test bundle file (hello.wbn) is generated with gen-bundle of > github.com/WICG/webpackage revision a3cef2c, which supports the new > bundle format except for the new index section structure. > > [1] WICG/webpackage#450 > [2] WICG/webpackage#454 > [3] WICG/webpackage#458 > > Bug: 969596 > Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487 > Reviewed-by: Robert Sesek <rsesek@chromium.org> > Reviewed-by: Tsuyoshi Horo <horo@chromium.org> > Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> > Cr-Commit-Position: refs/heads/master@{#684200} Bug: 969596 Change-Id: I1d68a3cb2975fc7856449180517f67131d7b7817 Tbr: toyoshim@chromium.org Tbr: kinuko@chromium.org Tbr: rsesek@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1736907 Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#684216}

This reverts commit d8c863a. Reason for revert: https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.linux/builders/Deterministic%20Linux/builds/24310 https://luci-milo.appspot.com/p/chromium/builders/ci/Deterministic%20Linux/24310 Compile failure, List of errors: ../../mojo/public/cpp/bindings/struct_ptr.h:56:18: error: no matching constructor for initialization of 'mojo::StructPtr<data_decoder::mojom::BundleMetadataParseError>::Struct' (aka 'data_decoder::mojom::BundleMetadataParseError') Original change's description: > BundledExchangesParser: Update the bundle format [1/2] > > This updates BundledExchangesParser to parse the new bundle format [1]. > > Changes: > - Updated the magic header bytes [2] > - Version field is added (we use implementation-specific version string > "b1\0\0", which matches gen-bundle's output [3]) > - Fallback URL (== Primary URL) field is added > - ParseMetadata() returns error type ("format error" or "version error") > and fallback URL if available > - Updated spec ref comments > > The structure change of the index section is not reflected yet. It will > be updated in the next CL. > > The test bundle file (hello.wbn) is generated with gen-bundle of > github.com/WICG/webpackage revision a3cef2c, which supports the new > bundle format except for the new index section structure. > > [1] WICG/webpackage#450 > [2] WICG/webpackage#454 > [3] WICG/webpackage#458 > > Bug: 969596 > Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487 > Reviewed-by: Robert Sesek <rsesek@chromium.org> > Reviewed-by: Tsuyoshi Horo <horo@chromium.org> > Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> > Cr-Commit-Position: refs/heads/master@{#684200} TBR=horo@chromium.org,kinuko@chromium.org,toyoshim@chromium.org,ksakamoto@chromium.org,rsesek@chromium.org Change-Id: I0e4c550179c4433022646f8ecc27cbdf4983ce87 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 969596 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1738036 Reviewed-by: Noel Gordon <noel@chromium.org> Commit-Queue: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#684219}

This reverts commit 3a99193. Reason for revert: This was already reverted at [1] and relanded with a fix at [2]. [1] https://chromium.googlesource.com/chromium/src/+/5bd3e578821d1daacdb92dd5180255a771bf6d6c [2] https://chromium.googlesource.com/chromium/src/+/4f518dc059bb7e0167335a3bb01b47d01bd0da80 Original change's description: > Revert "BundledExchangesParser: Update the bundle format [1/2]" > > This reverts commit d8c863a. > > Reason for revert: > https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.linux/builders/Deterministic%20Linux/builds/24310 > > > https://luci-milo.appspot.com/p/chromium/builders/ci/Deterministic%20Linux/24310 > > Compile failure, List of errors: > > ../../mojo/public/cpp/bindings/struct_ptr.h:56:18: error: no matching constructor for initialization of 'mojo::StructPtr<data_decoder::mojom::BundleMetadataParseError>::Struct' (aka 'data_decoder::mojom::BundleMetadataParseError') > > > Original change's description: > > BundledExchangesParser: Update the bundle format [1/2] > > > > This updates BundledExchangesParser to parse the new bundle format [1]. > > > > Changes: > > - Updated the magic header bytes [2] > > - Version field is added (we use implementation-specific version string > > "b1\0\0", which matches gen-bundle's output [3]) > > - Fallback URL (== Primary URL) field is added > > - ParseMetadata() returns error type ("format error" or "version error") > > and fallback URL if available > > - Updated spec ref comments > > > > The structure change of the index section is not reflected yet. It will > > be updated in the next CL. > > > > The test bundle file (hello.wbn) is generated with gen-bundle of > > github.com/WICG/webpackage revision a3cef2c, which supports the new > > bundle format except for the new index section structure. > > > > [1] WICG/webpackage#450 > > [2] WICG/webpackage#454 > > [3] WICG/webpackage#458 > > > > Bug: 969596 > > Change-Id: I95744ed00fdd09d369bb8648d1cdf62ca181d0dd > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715487 > > Reviewed-by: Robert Sesek <rsesek@chromium.org> > > Reviewed-by: Tsuyoshi Horo <horo@chromium.org> > > Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> > > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > > Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#684200} > > TBR=horo@chromium.org,kinuko@chromium.org,toyoshim@chromium.org,ksakamoto@chromium.org,rsesek@chromium.org > > Change-Id: I0e4c550179c4433022646f8ecc27cbdf4983ce87 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 969596 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1738036 > Reviewed-by: Noel Gordon <noel@chromium.org> > Commit-Queue: Noel Gordon <noel@chromium.org> > Cr-Commit-Position: refs/heads/master@{#684219} TBR=horo@chromium.org,kinuko@chromium.org,toyoshim@chromium.org,noel@chromium.org,ksakamoto@chromium.org,rsesek@chromium.org Change-Id: I27423f80338576b35506bb8f7e6010fc4c2d2ec7 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 969596 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1735802 Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org> Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#684221}

This updates BundledExchangesParser to parse the new strucure of the index section [1]. After this patch, BundledExchange's index metadata is returned as a map from URL to BundleIndexValue struct, which contains a variants string and one or more response locations. The test bundle file (hello.wbn) is generated with gen-bundle of github.com/WICG/webpackage revision 31e1847, which supports the new index section structure. [1] WICG/webpackage#450 [2] WICG/webpackage#466 Bug: 969596 Change-Id: I026da3e325eec702678a1136181be8f5f3308c68 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1722833 Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#684631}

The index section of bundle maps URLs to a Variants value + a list of the responses for each possible Variant-Key (WICG#450), but before this patch gen-bundle could not generate a bundle that have multiple variants for single URL. This patch teaches indexSection.Finalize() to generate entries with non-empty variants-value, based on the responses' Variant and Variant-Key headers [1]. [1] https://tools.ietf.org/html/draft-ietf-httpbis-variants-05

The index section of bundle maps URLs to a Variants value + a list of the responses for each possible Variant-Key (WICG#450). But before this patch gen-bundle could not generate a bundle that have multiple variants for single URL. This patch teaches indexSection.Finalize() to generate entries with non-empty variants-value, based on the responses' Variant and Variant-Key headers [1]. [1] https://tools.ietf.org/html/draft-ietf-httpbis-variants-05

The index section of bundle maps URLs to a Variants value + a list of the responses for each possible Variant-Key (#450). But before this patch gen-bundle could not generate a bundle that have multiple variants for single URL. This patch teaches indexSection.Finalize() to generate entries with non-empty variants-value, based on the responses' Variant and Variant-Key headers [1]. [1] https://tools.ietf.org/html/draft-ietf-httpbis-variants-05

jyasskin added 6 commits July 4, 2019 21:15

Revamp the bundle index to use Variants.

6f0e473

Fix the variant-keys order examples to make sense in context.

b63d3e7

Add a signatures section to vouch for bundle contents.

2db2615

Add the fallback URL to bundles.

fd0f01c

Add a mime type version, and make the Variants header value a byte st…

b5430bb

…ring.

Add some infrastructure to only redirect to the fallback URL on certa…

dbb60f6

…in kinds of errors.

jyasskin requested review from irori and kinu July 5, 2019 05:01

irori approved these changes Jul 8, 2019

View reviewed changes

kinu approved these changes Jul 8, 2019

View reviewed changes

Fix irori's and kinu's comments.

a39514f

jyasskin merged commit 744dffc into WICG:master Jul 8, 2019

jyasskin deleted the update-bundle-format branch July 8, 2019 18:41

irori reviewed Jul 10, 2019

View reviewed changes

draft-yasskin-wpack-bundled-exchanges.md Show resolved Hide resolved

irori mentioned this pull request Jul 16, 2019

go/bundle: Add support for multiple versions #458

Merged

irori mentioned this pull request Jul 18, 2019

go/bundle: Add support for PrimaryURL (aka fallbackURL) #461

Merged

irori mentioned this pull request Jul 24, 2019

go/bundle: Add support for the new index section format #466

Merged

irori mentioned this pull request Nov 28, 2019

gen-bundle: Add support for Variants #539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the bundle format #450

Update the bundle format #450

jyasskin commented Jul 5, 2019

irori left a comment

irori Jul 8, 2019

jyasskin Jul 8, 2019

irori Jul 8, 2019

jyasskin Jul 8, 2019

irori Jul 8, 2019

jyasskin Jul 8, 2019

kinu left a comment

kinu Jul 8, 2019

jyasskin Jul 8, 2019

kinu Jul 8, 2019

jyasskin Jul 8, 2019

		@@ -739,6 +881,14 @@ at <https://www.iana.org/assignments/media-types>.

		* Required parameters: N/A

Update the bundle format #450

Update the bundle format #450

Conversation

jyasskin commented Jul 5, 2019

irori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment