Closed Bug 1002932 Opened 10 years ago Closed 10 years ago

[email] cookie cache no longer working with GAIA_OPTIMIZE=1

Categories

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jrburke, Assigned: jrburke)

Details

(Keywords: regression)

Attachments

(1 file)

With the email app now uglifying the JS contents, the CACHE_VERSION variable in html_cache_restore.js is mangled to another name, so the digest stamping of the cache value during gaia builds no longer works.

This bug was introduced when bug 960343 was fixed. Funny enough, I thought bug 960343 would have been landed for 1.3T, but it did not go there or 1.3 (both are using unminified sources), so just 1.4 and master are affected by this change.

The problem is the bug is very subtle: when minification is working across all files (which recently it did not for mail_app which includes part of the reference to the cache version, filed bug 1002920 to track that), then both cache versions are at value of "1". So it will appear to work for the first release that just goes out with bug 960343, but further updates (if this bug fix is not done) will result in a bad cache.

So, hard to test for the failure, but eventually we would get bugs about the UI getting all weird or stuck over releases as the cache HTML would not match the expected DOM.

Asking for 1.4 blocking given the subtleness of the problem that compounds over time, and what should be an easily verifiable fix for the build process, at least when looking at the built output files -- the CACHE_VERSION ID should be stamped correctly in mail_app and html_cache_restore.
Attached file GitHub pull request
Switch to using a global, HTML_COOKIE_CACHE_VERSION, set up in html_cache_restore.js, and only modify that one global in that one file.

I have a strong aversion to globals, but since this value is used before we have a module system available, and because we want the value to be the same there as well as in the html_cache.js file, the global makes sense. This also avoids issues with minification trying to rename it, and means we only need to stamp one file in make_gaia_shared.js.

How I verified the fix:

   make install-gaia GAIA_OPTIMIZE=1 APP=email

then in build_stage/email, I make sure that js/mail_app.js still has a HTML_COOKIE_CACHE_VERSION reference in the 'html_cache' module (so it survived minification) and that in js/html_cache_restore.js, the HTML_COOKIE_CACHE_VERSION value is stamped with the digest generated from the source files.

It also worked on the device. :)

If you want to test this locally, you will want to get the l10n.js fix that is in bug 1002920. This bug is not dependent on that fix, however, to see that the constant survives minification, that fix is necessary for mail_app.js to be minified on master at the moment.
Attachment #8414262 - Flags: review?(bugmail)
Blocking 1.4 for regression.
blocking-b2g: 1.4? → 1.4+
Keywords: regression
Comment on attachment 8414262 [details] [review]
GitHub pull request

r=asuth by inspection.  Great find on this!

Maybe we should add a console.log or a dump in the (version !== HTML_COOKIE_CACHE_VERSION) case to make it more obvious when we're failing to restore the cache for a version mismatch reason.  That's the (unexpected) slow path, so the overhead of the log/dump should be trivial.  I do realize this will come before any of our fancier console.log shims have loaded, so it won't look quite as cool.
Attachment #8414262 - Flags: review?(bugmail) → review+
Merged to gaia master:
https://github.com/mozilla-b2g/gaia/commit/72678612a5556f28ab607c57e7c6a02c5f10e941

from pull request:
https://github.com/mozilla-b2g/gaia/pull/18773

Per feedback, I included a console.log for the case where we do not decide to use the cache, so that if this somehow breaks in the future it will be more obvious.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: