600995 - Relative URI handling confused by slashes in form records

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[juan becerra [:juanb]]

Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100930 Firefox/4.0b7pre

I was installing a new Linux VM, and I wanted to try Sync using my personal account. I looked in about:sync-log out of curiosity and saw lots of errors like the one in the summary.

See about:sync-log and about:sync-log.1 attachments.

The information was originally sync'd using Namoroka with Sync 1.5b6

Comment 1

[juan becerra [:juanb]]

Attachment: about sync-log

Comment 2

[juan becerra [:juanb]]

Attachment: about sync-log.1

Excluding bookmark synchronization part

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 479939 [details]
about sync-log.1

>.....
>2010-09-30 14:41:15	Collection           DEBUG	GET success 200 https://phx-sync074.services.mozilla.com/1.0/juanb/storage/bookmarks?full=1
>2010-09-30 14:41:15	Engine.Bookmarks     INFO	Records: 3488 applied, 0 reconciled, 0 left to fetch
>2010-09-30 14:41:15	Engine.Bookmarks     INFO	Uploading all of 13 records
>2010-09-30 14:41:15	Collection           DEBUG	POST Length: 8995
>2010-09-30 14:41:16	Collection           DEBUG	POST success 200 https://phx-sync074.services.mozilla.com/1.0/juanb/storage/bookmarks
>2010-09-30 14:41:16	Collection           DEBUG	DELETE success 200 https://phx-sync074.services.mozilla.com/1.0/juanb/storage/bookmarks?ids={ed2c27f7-ea41-41ea-a9d6-bf951235da7b}1,{ed2c27f7-ea41-41ea-a9d6-bf951235da7b}11,{ed2c27f7-ea41-41ea-a9d6-bfâÃâ¬Ã¦
>2010-09-30 14:41:17	Engine.Bookmarks     DEBUG	Total (ms): sync 118388, processIncoming 116823, uploadOutgoing 1083, syncStartup 135, syncFinish 31, createRecord 32, findDupe 106, handleDupe 23, resetLastSync 0, deleteId 0, reconcile 3870, resetClient 0

I'm guessing bookmarks and the other engines synced without these error messages?

>2010-09-30 14:41:17	Net.Resource         DEBUG	GET success 200 https://phx-sync074.services.mozilla.com/1.0/juanb/storage/crypto/forms
>2010-09-30 14:41:17	Engine.Forms         DEBUG	Engine syncIDs: a-tY4We0Ax,iijrvFEi62
>2010-09-30 14:41:17	Engine.Forms         DEBUG	Resetting forms last sync time
>2010-09-30 14:41:17	Engine.Forms         DEBUG	First sync, uploading all items
>2010-09-30 14:41:17	Engine.Forms         INFO	0 outgoing items pre-reconciliation
>2010-09-30 14:41:18	Net.Resource         DEBUG	GET fail 400 https://phx-sync074.services.mozilla.com/1.0/juanb/crypto/forms
>2010-09-30 14:41:18	Engine.Forms         WARN	Error processing record: meta is null JS Stack trace: CryptoWrapper_decrypt([object Object])@crypto.js:84 < ([object Object])@engines.js:471 < innerBind([object Object])@util.js:904 < ()@collection.js:160 < Channel_onDataAvail([object XPCWrappedNative_NoHelper],null,[object XPCWrappedNative_NoHelper],3839,111472)@resource.js:372

It's missing the "storage/" part of the URL here. This sounds like a problem with the relative URL stuff. It only seems to happen for 56 of your 770 form records. My guess is that these are form records that have slashes in their GUIDs, which utterly confuses our relative URI handling. It created the encryption record as "../../crypto/forms" (whereas "../crypto/forms" would've been correct) which then got applied to the stub URI we provide for new records, eating the additional "storage/" part of the URI.

Comment 4

[Philipp von Weitershausen [:philikon]]

I believe we should do two things here:

1. Ignore whatever 'encryption' property we find on the record. They might be wrong as we've seen here, and the engine knows it already anyway. So let's just short-circuit that to engine's value.

2. Generate proper relative URIs and record IDs. Slashes in record IDs feels like asking for disaster, so perhaps the form engine should just translate slashes to something safer.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): short-circuit record.encryption

Here's a patch for 1)

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): short-circuit record.encryption

Implement fallback that actually makes sense: Try to decrypt with engine.cryptoMetaURL, if that fails, use item.encryption.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 480595 [details] [diff] [review]
Part 1 (v2): short-circuit record.encryption

this is better, please note the bug # in the comment about relative URI confusions.

Comment 8

[Philipp von Weitershausen [:philikon]]

Landed Part 1: http://hg.mozilla.org/services/fx-sync/rev/57142f068ebd

We probably want to fix part 2 in satchel itself. Filed bug 601730 for that. Resolving this one.

Comment 9

[Tracy Walker [:tracy]]

verified with 1.5b7 on nightly Minefield builds

Comment 10

[Tracy Walker [:tracy]]

This only seems to be partially fixed.  Form records with slashes in them can still get stuck by syncing client B before 1.5b7 is installed.  The only way I found to make this work right is to not sync on each client 'til 1.5b7 is installed.

scenario with bug:
1) client A with 1.5b7, sync it 
2) client B without 1.5b7, sync it. observe bug in slash containing form items
3) upgrade client B with 1.5b7, sync it. bug remains

scenario without bug:
1) client A with 1.5b7, sync it 
2) fresh profile on client B with 1.5b7 installed before ever syncing, sync it.  no bug, slashed form items sync fine.

should this be reopened? or is the first scenario unavoidable?

Comment 11

[Tony Chung [:tchung]]

Just tested this against Fennec 4.0 beta 1 build 2, and i'm not seeing the fix there.  

about:sync-log shows:
Net.Resource     DEBUG   fail 400 https://phx-sync215.services.mozilla.com/1.0/<myDeviceName>/crypto/forms
Engine.Forms     WARN    Error processing record: meta is null JS Stack trace: CrytpWrapper_decrypt([object Object])@crypto.js:84

PK says it should be fixed in today's nightly though.  will try this again to verify.