Closed Bug 1309699 Opened 4 years ago Closed 4 years ago

Backout bug 1279568 to address content crashes in Beta50 only

Categories

(Core :: DOM: Security, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed
firefox51 --- wontfix
firefox52 --- wontfix

People

(Reporter: ritu, Assigned: jhao)

References

Details

Attachments

(1 file, 6 obsolete files)

Filing this bug to address this comment https://bugzilla.mozilla.org/show_bug.cgi?id=1289001#c39 which suggests backing out bug 1279568 to address content crashes in Beta50. 

Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1279568#c46
I would like the backout or conditional pref change mentioned here https://bugzilla.mozilla.org/show_bug.cgi?id=1289001#c44 to land on moz-beta branch before I gtb 50.0b7 tomorrow noon PST.

If this happens tonight PST or Taipei/Europe time, please work with :tomcat to get this landed on Nightly, and :sylvestre/:gchang to get Aurora/Beta patch uplift approved and then
Flags: needinfo?(tanvi)
Flags: needinfo?(sledru)
Flags: needinfo?(gchang)
Flags: needinfo?(cbook)
Hi Jonathan,

Do you have time to do this bug today (Taipei's Thursday)?  You can send review request to baku or ehsan (whoever can get to it sooner).

We need to do one of two things:

1) Add a pref, privacy.usercontext.about_newtab_segregation.enabled.  Take your patches in bug 1279568 that caused newtab to use usercontextid=5 and make them conditional on that pref.  When you set the value of the pref, make it false for everything but Nightly.

In the future, we can consolidate privacy.usercontext.about_newtab_segregation and privacy.usercontext.enabled into one pref, but for now I think we need two.

This solution would need to land on Nightly, Aurora, and Beta.

2) Back out your patches in bug 1279568 from Beta only.

I prefer number 1, but if you run into problems, then do number 2.

Once the code is done and reviewed, tell tomat, sylvestre, and/or gchang where the patches should land (all channels, or just beta).

Thanks!
Assignee: nobody → jhao
Flags: needinfo?(tanvi)
Summary: Backout bug 1279568 to address content crashes in Beta50 → Backout or add pref for bug 1279568 to address content crashes in Beta50
I'm on it.
Status: NEW → ASSIGNED
Hi Baku, would you review this patch, please?
Attachment #8800540 - Flags: review?(amarchesini)
Attachment #8800540 - Flags: review?(amarchesini) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/9a047a39cb2f
Add pref for bug 1279568 to address content crashes in Beta50. r=baku, a=tomcat
https://hg.mozilla.org/mozilla-central/rev/9a047a39cb2f7e4f97a8b5c0bccc3096efd0aa05

landed this directly on central to get nightly coverage
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(cbook)
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Attached patch Rebased patch for aurora. (obsolete) — Splinter Review
Attachment #8800563 - Flags: review+
Comment on attachment 8800540 [details] [diff] [review]
Add pref for bug 1279568 to address content crashes in Beta50.

[Triage Comment] adds pref to fix content crashes in beta50, needed for b7
Attachment #8800540 - Flags: approval-mozilla-beta+
Attachment #8800540 - Flags: approval-mozilla-aurora+
clearing ni for sledru/gchang
Flags: needinfo?(sledru)
Flags: needinfo?(gchang)
I should've turn on the new pref in that test.
Flags: needinfo?(jhao)
Hi Mark, would you please review this patch?  We had to turn off the loading thumbnails in a separate container thing to avoid crashes, but it breaks the no_cookie_stored test.
Attachment #8800646 - Flags: review?(markh)
Comment on attachment 8800646 [details] [diff] [review]
Fix the test (also applies to aurora and beta)

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

stealing the review!
Attachment #8800646 - Flags: review?(markh) → review+
Thanks for the rescue, Ehsan! Because I just realized what Mark's time zone is and he probably won't see this before tomorrrow noon PST.

Add r=ehsan to commit message.
Attachment #8800699 - Flags: review+
Attachment #8800646 - Attachment is obsolete: true
Comment on attachment 8800699 [details] [diff] [review]
Fix the test (also applies to aurora and beta)

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Blocking the uplift of the other patch which solves a crash.
[Describe test coverage new/current, TreeHerder]: It's a test itself.
[Risks and why]: None.  It's just a test.
[String/UUID change made/needed]: None.
Attachment #8800699 - Flags: approval-mozilla-beta?
Attachment #8800699 - Flags: approval-mozilla-aurora?
Comment on attachment 8800699 [details] [diff] [review]
Fix the test (also applies to aurora and beta)

Fixing a test for a patch needed in beta
Attachment #8800699 - Flags: approval-mozilla-beta?
Attachment #8800699 - Flags: approval-mozilla-beta+
Attachment #8800699 - Flags: approval-mozilla-aurora?
Attachment #8800699 - Flags: approval-mozilla-aurora+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/559fa765dbf1
Turn on the new pref in browser_thumbnails_bg_no_cookies_stored.js. r=ehsan
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/829c16e72fb7
Spell the preference name correctly so it works a=me
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b10e48ea5d20
Make super-triple-extra sure we're using the right preference name a=me CLOSED TREE
I embarrassed myself.  I should've notice the spelling error earlier.

Thank you, Ryan and Wes.
Tests on aurora and beta are still failing like https://treeherder.mozilla.org/logviewer.html#?job_id=3846062&repo=mozilla-aurora even after getting the preference spelled correctly.
Flags: needinfo?(jhao)
NI Tanvi and Baku in case they can get to this sooner. We really need to understand if the test failure is a code bug/new regression due to this change or a test related problem. I would like to push 50.0b7 with this conditional pref tomm am PST. I'd appreciate a prompt reply.
Flags: needinfo?(tanvi)
Flags: needinfo?(amarchesini)
Backed out all four beta csets in https://hg.mozilla.org/releases/mozilla-beta/rev/688d16cc31ada89128bcf1ee0fd81391757f46ac (without looking first, sorry, but, let's go with one push which has actually been tested and will actually work, rather than a stream of wild guesses).
FWIW, this looks to me like a test-only issue - bug 1279568 landed some additional tests to demonstrate the "old way" of doing background captures would cause cookies to be stored via XHR requests or scripts referencing document.cookie, and I believe it is these tests which are failing (although the taskcluster stack traces are useless so I can't be 100% sure, but I strongly suspect that is the case)
oops - bugzilla is dumb
Attachment #8800540 - Attachment is obsolete: true
Attachment #8800563 - Attachment is obsolete: true
Attachment #8800699 - Attachment is obsolete: true
Attachment #8800939 - Attachment is obsolete: true
Comment on attachment 8800942 [details] [diff] [review]
Backout changes in Bug 1279568.

Hi Mark, would you review this patch that backout all the changes in bug 1279568?
Flags: needinfo?(jhao)
Attachment #8800942 - Flags: review?(markh)
Attachment #8800942 - Flags: review?(markh) → review+
Rebased it upon previous backouts. r=markh
Attachment #8800965 - Flags: review+
Attachment #8800942 - Attachment is obsolete: true
Comment on attachment 8800965 [details] [diff] [review]
Backout changes in Bug 1279568.

Requesting to uplift to beta, and preferably NOT to nightly and aurora.  We'd like to keep the newtab segregation in aurora and nightly.  We're trying to fix the crash in the right way in bug 1289001. This backout patch is just a work-around for beta.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: this patch solves a crash in beta.
[Describe test coverage new/current, TreeHerder]: toolkit/components/thumbnails/test
[Risks and why]: Minimum.  No new code added.  Only backing out previous bugs.
[String/UUID change made/needed]: None
Attachment #8800965 - Flags: approval-mozilla-beta?
Comment on attachment 8800965 [details] [diff] [review]
Backout changes in Bug 1279568.

OK, let's try it again.
Should be in 50 beta 8
Attachment #8800965 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
What's the status of this bug then? Should we call 51/52 wontfix in favor of the work happening in bug 1289001? This is turning into a mess to track :(
Flags: needinfo?(jhao)
This bug should only land in Beta 50.  In Firefox 51 and 52, we should land the pref instead of the full backout; that work will happen in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1310112.
Flags: needinfo?(tanvi)
I was explicitly requested to land attachment 8800699 [details] [diff] [review] on trunk as well yesterday, but OK.
Flags: needinfo?(jhao)
Flags: needinfo?(amarchesini)
Target Milestone: mozilla52 → mozilla50
Summary: Backout or add pref for bug 1279568 to address content crashes in Beta50 → Backout bug 1279568 to address content crashes in Beta50 only
Follow-up: Bug 1310112
See Also: → 1310112
You need to log in before you can comment on or make changes to this bug.