Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 8 years ago

Once the stability issues have been fixed, the API should be enabled again.

	Nicholas Nethercote [inactive]
	Comment 1 • 8 years ago

Most of the crash signatures have "IntersectionObserver" in them and so are obviously related. But there were a few ones that weren't like that. From bug 1317415:

> [@ nsGenericHTMLElement::QueryInterface ]
> [@ RtlDispatchException | KiUserExceptionDispatcher ]
> [@ NS_TableDrivenQI ]

	Tobias Schneider [:tobytailor] Assignee
	Comment 2 • 8 years ago

Once Bug 1322717 is landed, this patch flips the pref to enable the IntersectionObserver API.

	Nicholas Nethercote [inactive]
	Comment 3 • 8 years ago

Tobias: are you confident that the patch in bug 1322717 will fix all the crashes, with various signatures, that we've seen? Have you been able to reproduce more than one of the crash signatures yourself? Thanks.

	Tobias Schneider [:tobytailor] Assignee
	Comment 4 • 8 years ago

Yes, Im confident and yes I was able to reproduce them myself (took me a couple of days surfing around on a Windows machine tho). I was wondering about why most reports came from Windows machines, but I think thats just because of the larger share of Nightly users on that platform. I successfully reproduced it on Mac OS X too. It really requires certain circumstances to happen for those crashes which makes it difficult to deterministically reproduce it with a test.

	Nicholas Nethercote [inactive]
	Comment 5 • 8 years ago

Good to hear! And yes, the Windows population is *much* larger than Mac and Linux on all channels.

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 8 years ago

Landing this with an implicit r=smaug given the belief that the crashes are indeed fixed now.

	Pulsebot
	Comment 7 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ffd7d13eb96
Enable IntersectionObserver. r=smaug

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 8 • 8 years ago

Backed out for not enabling IntersectionObserverEntry in the whitelist of test_interfaces.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/83882d91f759155dad8fdb8c68af0e6d65906b93

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8ffd7d13eb961a3f6df90e17408534790f58bf89
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40463921&repo=mozilla-inbound

dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface IntersectionObserverEntry to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals)

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 9 • 8 years ago

r=smaug to add IntersectionObserver to the test_interfaces.html list.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 10 • 8 years ago

but but, doesn't one need to also enable the tests again too, the one which bug 1320704 disabled.

	Pulsebot
	Comment 12 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc211e6cb0c1
Enable IntersectionObserver r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed256c5c7d23
Enable IntersectionObserver Tests. r=smaug

	Carsten Book [:Tomcat]
	Comment 13 • 8 years ago

backed out for test failurs like https://treeherder.mozilla.org/logviewer.html#?job_id=40567662&repo=mozilla-inbound

	Pulsebot
	Comment 14 • 8 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e171b4e26d
Backed out changeset ed256c5c7d23 
https://hg.mozilla.org/integration/mozilla-inbound/rev/48dbfd4172e1
Backed out changeset dc211e6cb0c1 for failing on own test

	Tobias Schneider [:tobytailor] Assignee
	Comment 15 • 8 years ago

Keep disabled on android.

	Pulsebot
	Comment 16 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae78eda5a0c
Enable IntersectionObserver. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/016b87fe9145
Enable IntersectionObserver tests. r=smaug

	Carsten Book [:Tomcat]
	Comment 17 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3ae78eda5a0c
https://hg.mozilla.org/mozilla-central/rev/016b87fe9145

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 18 • 8 years ago

Still getting crashes with this enabled:

STR:
1.  Visit: http://www.foxnews.com/tech/2016/12/16/incredible-drone-footage-shows-killer-whales-eating-living-shark.html
2.  Scroll down a bit to the video 
3.  Crash

c-r https://crash-stats.mozilla.com/report/index/7167d7cd-c0b2-48d0-99c2-36a5c2161217

	Andrew McCreight [:mccr8]
	Comment 19 • 8 years ago

Backed out for causing bug 1324209 (which is the top crash by a large margin on the latest Nightly):
https://hg.mozilla.org/integration/mozilla-inbound/rev/1016d96bdabb1522645aa97d18a3e438a3cc35b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f22c6dc53276d9cb1e7365e47dc52657fac

	Phil Ringnalda (:philor)
	Comment 20 • 8 years ago
backout bugherder

https://hg.mozilla.org/mozilla-central/rev/1016d96bdabb
https://hg.mozilla.org/mozilla-central/rev/f22c6dc53276

	Pulsebot
	Comment 21 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3ce9502a04
Enable IntersectionObserver. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/394aea8026a3
Enable IntersectionObserver tests. r=smaug

	Phil Ringnalda (:philor)
	Comment 22 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1e3ce9502a04
https://hg.mozilla.org/mozilla-central/rev/394aea8026a3

	Nicholas Nethercote [inactive]
	Comment 23 • 7 years ago

Can one of tschneider, RyanVM or smaug please back this out ASAP? Given the history of this new API -- it's been the top cause of crashes in Nightly on three different occasions now -- I think a disable-first-and-ask-questions-later approach is appropriate.

bsmedberg has been talking about writing test plans for new features to ensure a minimum standard of quality for new features. (https://wiki.mozilla.org/QA/Test_Plan_Template is one relevant document, and there are undoubtedly others.) Perhaps a test plan should be created for this feature, and an appropriate level of testing done, before we re-enable it again on Nightly.

	Tobias Schneider [:tobytailor] Assignee
	Comment 24 • 7 years ago

RyanVM can you please back this out until I have the crashes fixed?

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 25 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3346ef79c433e6f01f6fb392a9d909a888af9a38
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a71a4ea34d61a51fdee2836a7ea4663bc19f06

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 7 years ago

Was comment 23 ever addressed regarding a test plan? FWIW, I'm inclined to agree with Nick's assessment here.

	Tobias Schneider [:tobytailor] Assignee
	Comment 27 • 7 years ago

I agree with what Nick said and that we need a test plan for this. Actually, I'm already working on defining one. The latest crash was a regression introduces by myself, trying to fix another crash. Totally my fault. I would like to give this another shot. I tested affected websites for a couple of days without running into any further issues. I will have an eye on this and if needed asking for blackout a soon as crashes should come in.

	Pulsebot
	Comment 28 • 7 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8beb4b722b73
Enable IntersectionObserver. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9103460d1a
Enable Tests. r=smaug

	Carsten Book [:Tomcat]
	Comment 29 • 7 years ago

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=67922908&repo=mozilla-inbound

	Pulsebot
	Comment 30 • 7 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8495cffc78
Backed out changeset 1f9103460d1a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/45038edd8121
Backed out changeset 8beb4b722b73 for errors like in test_intersectionobservers.html

	Tobias Schneider [:tobytailor] Assignee
	Comment 31 • 7 years ago

Rebased patch.

	Tobias Schneider [:tobytailor] Assignee
	Comment 32 • 7 years ago

The real one.

	Tobias Schneider [:tobytailor] Assignee
	Comment 33 • 7 years ago

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=677470bff8e880d5b732fb6f43a2374243f3f0e6&selectedJob=68189945

	Pulsebot
	Comment 34 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd23e1fc730b
Enable IntersectionObserver. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/31511bed6415
Enable IntersectionObserver Tests. r=smaug

	Phil Ringnalda (:philor)
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cd23e1fc730b
https://hg.mozilla.org/mozilla-central/rev/31511bed6415

	Tobias Schneider [:tobytailor] Assignee
	Comment 36 • 7 years ago

Ryan, please back out again due to newly reported crashes.

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 7 years ago

By my count, this is now the 4th time this has landed and been backed out for stability issues (and is now going to slip the 53 release as well). I'm sorry for the bluntness here, but we clearly have insufficient test coverage of this feature, and that needs to block this being re-enabled. Can we get some targeted fuzzing, code coverage results, whatever to figure out what we're missing so that this can be turned back on when we're truly confident in its readiness for doing so? At an absolute bare minimum, we clearly need targeted testing from QA before flipping the pref again for nightly users.

https://hg.mozilla.org/mozilla-central/rev/d5e37c0a776f1f2c21ddac4612529f819e13733b

	Jet Villegas (inactive)
	Comment 38 • 7 years ago

Tobias: Please set me to r? before landing this one again. Thx!

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 39 • 7 years ago

Might we worth to explain the ownership model of InterserctionObserver to some DOM folks who have dealt with GC and CC.
Happy to help here.

	Calixte Denizet (:calixte)
	Comment 40 • 7 years ago

Right now we've 1087 crashes in nightly with build-id 20170122030212.

	Tyson Smith [:tsmith]
	Comment 41 • 7 years ago

:truber is working on adding support to existing fuzzers to get some fuzz coverage.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 42 • 7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #37)
> By my count, this is now the 4th time this has landed and been backed out
> for stability issues (and is now going to slip the 53 release as well). I'm
> sorry for the bluntness here, but we clearly have insufficient test coverage
> of this feature, and that needs to block this being re-enabled. Can we get
> some targeted fuzzing, code coverage results, whatever to figure out what
> we're missing so that this can be turned back on when we're truly confident
> in its readiness for doing so? At an absolute bare minimum, we clearly need
> targeted testing from QA before flipping the pref again for nightly users.

I think the main thing that's going to help here is more review from (and discussion with) folks who have dealt with these issues before, a la comment 53.  Would be great to see that happen in time for 54.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 43 • 7 years ago

er, s/comment 53/comment 39/.  Sorry.

	Jet Villegas (inactive)
	Comment 44 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #42)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #37)
> > By my count, this is now the 4th time this has landed and been backed out
> > for stability issues (and is now going to slip the 53 release as well). I'm
> > sorry for the bluntness here, but we clearly have insufficient test coverage
> > of this feature, and that needs to block this being re-enabled. Can we get
> > some targeted fuzzing, code coverage results, whatever to figure out what
> > we're missing so that this can be turned back on when we're truly confident
> > in its readiness for doing so? At an absolute bare minimum, we clearly need
> > targeted testing from QA before flipping the pref again for nightly users.
> 
> I think the main thing that's going to help here is more review from (and
> discussion with) folks who have dealt with these issues before, a la comment
> 53.  Would be great to see that happen in time for 54.

Yes, these discussions are happening and will continue. Another priority is to formalize a Test Plan for the feature. Tobias will share a link when that's ready for wider review.

	Tobias Schneider [:tobytailor] Assignee
	Comment 45 • 7 years ago

See https://wiki.mozilla.org/QA/IntersectionObserver

	Tobias Schneider [:tobytailor] Assignee
	Comment 47 • 7 years ago

Put together a document describing the lifetime management of the Intersection Observer API: https://docs.google.com/document/d/1bHK0_aTVyi5cBYODRYyuK-wQdJ2bTY6QPZu4F0AF0Io/edit?usp=sharing

	Jet Villegas (inactive)
	Comment 48 • 7 years ago

Comment on attachment 8825915 [details] [diff] [review]
Enable IntersectionObserver

Experimental deployment looks good. We'll go to 100% on the test group and then enable this feature. After it's in Nightly for a bit, and we don't take more code changes, please flag this patch for uplift to FF54. Thanks!

	Pulsebot
	Comment 49 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/718fb66559f7
Enable IntersectionObserver and tests. r=smaug, r=jet

	Carsten Book [:Tomcat]
	Comment 50 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/718fb66559f7

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 51 • 7 years ago

I just backed this out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35af33f46d9499052b4c1ccdee93907f602b6698

we are almost failing once/push for the new test case- please revisit and show some try pushes/etc. I would recommend retriggering a few times mochitest-e10s-1.  More details in bug 1313972.

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 7 years ago

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/35af33f46d94

	Tobias Schneider [:tobytailor] Assignee
	Comment 53 • 7 years ago

With Bug 1313972 being fixed, let's re-land this.

	Ryan VanderMeulen [:RyanVM]
	Comment 54 • 7 years ago

Can we verify that on Try first?

	Tobias Schneider [:tobytailor] Assignee
	Comment 55 • 7 years ago

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa08a5a24e2a38861ae19dae375d00d9a774a189

	Ryan VanderMeulen [:RyanVM]
	Comment 56 • 7 years ago

Lovely, thanks :). Don't hesitate to include recent Try links in the future when requesting checkin as well.

	Pulsebot
	Comment 57 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/83041db77fcf
Enable IntersectionObserver and tests. r=smaug, r=jet

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 58 • 7 years ago

Backed out for frequently failing test_intersectionobservers.html:

https://hg.mozilla.org/integration/autoland/rev/edbc280464588c955d0b184e37b661cc5985ccc7

A push with failures: https://treeherder.mozilla.org/logviewer.html#?job_id=91441042&repo=autoland
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=91441042&repo=autoland

[task 2017-04-13T20:49:26.669319Z] 20:49:26     INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | boundingClientRect matches target.getBoundingClientRect() for an element inside an iframe [observe subframe] 
[task 2017-04-13T20:49:26.671435Z] 20:49:26     INFO - Buffered messages finished
[task 2017-04-13T20:49:26.674169Z] 20:49:26     INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_intersectionobservers.html | rootBounds should is set to null for cross-origin observations [observe subframe] 
[task 2017-04-13T20:49:26.676908Z] 20:49:26     INFO -     get be/fn.ok@dom/base/test/test_intersectionobservers.html:97:13
[task 2017-04-13T20:49:26.678630Z] 20:49:26     INFO -     window.onmessage@dom/base/test/test_intersectionobservers.html:973:11
[task 2017-04-13T20:49:26.680859Z] 20:49:26     INFO -     EventHandlerNonNull*@dom/base/test/test_intersectionobservers.html:972:9
[task 2017-04-13T20:49:26.682889Z] 20:49:26     INFO -     next@dom/base/test/test_intersectionobservers.html:144:9
[task 2017-04-13T20:49:26.684948Z] 20:49:26     INFO -     next/<@dom/base/test/test_intersectionobservers.html:146:11
[task 2017-04-13T20:49:26.686916Z] 20:49:26     INFO -     io<@dom/base/test/test_intersectionobservers.html:959:11
[task 2017-04-13T20:49:26.689039Z] 20:49:26     INFO -     IntersectionCallback*@dom/base/test/test_intersectionobservers.html:953:14
[task 2017-04-13T20:49:26.691277Z] 20:49:26     INFO -     next@dom/base/test/test_intersectionobservers.html:144:9
[task 2017-04-13T20:49:26.693733Z] 20:49:26     INFO -     next/<@dom/base/test/test_intersectionobservers.html:146:11
[task 2017-04-13T20:49:26.695537Z] 20:49:26     INFO -     targetEl4.onload/<@dom/base/test/test_intersectionobservers.html:942:13

	Tobias Schneider [:tobytailor] Assignee
	Comment 59 • 7 years ago

Mh, did that Try task run with the patch from Bug 1313972?

	Pulsebot
	Comment 60 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/25b64d100bda
Enable IntersectionObserver and tests. r=smaug, r=jet

	Phil Ringnalda (:philor)
	Comment 61 • 7 years ago

Ah, Groundhog Day is late this year, apparently.

Backed out again in https://hg.mozilla.org/integration/autoland/rev/2a494e1f39e7

	Jet Villegas (inactive)
	Comment 62 • 7 years ago

(In reply to Phil Ringnalda (:philor) from comment #61)
> Ah, Groundhog Day is late this year, apparently.
> 
> Backed out again in
> https://hg.mozilla.org/integration/autoland/rev/2a494e1f39e7

The failing run:
https://hg.mozilla.org/integration/autoland/file/25b64d100bda232ad31a4e72fdb7b92e3220955f/dom/base/test

...doesn't seem to have the required test fixes already in m-c from bug 1313972:
https://hg.mozilla.org/mozilla-central/rev/05af3a08fb6f

Are the integration branches out of sync?

	Ryan VanderMeulen [:RyanVM]
	Comment 63 • 7 years ago

No, the reason this got backed out was due to a timeout this time:
https://treeherder.mozilla.org/logviewer.html#?job_id=91517297&repo=autoland

That's from a run after the test fix was merged to autoland. Comment 60 was literally the push prior to said merge arriving.

	Tobias Schneider [:tobytailor] Assignee
	Comment 64 • 7 years ago

If didn't have have the latest patch from Bug 1313972 then the timeout was most likely from a regression introduced by the earlier patch in that same bug. The latest patch was supposed to revert that.

	Ryan VanderMeulen [:RyanVM]
	Comment 65 • 7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #63)
> That's from a run after the test fix was merged to autoland.

The timeouts started on autoland *after* bug 1313972 was merged there, per the above comment.

	Tobias Schneider [:tobytailor] Assignee
	Comment 66 • 7 years ago

Was that the first try run after the patches landed? I see a couple other timeouts, which might be just coincidence tho.

	Ryan VanderMeulen [:RyanVM]
	Comment 67 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=linux%20m(2)&tochange=2a494e1f39e78f9bb25681a3b6616c6697e48685

	Tobias Schneider [:tobytailor] Assignee
	Comment 68 • 7 years ago

Latest try run finally looking good after Bug 1354163, 1324135 and 1313972 being fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b443f737b08d934457b900e83707dd021c306cf

	Jet Villegas (inactive)
	Comment 69 • 7 years ago

Comment on attachment 8819102 [details] [diff] [review]
Enable Tests. r=smaug


> [test_intersectionobservers.html]
>-skip-if = true # Track Bug 1320704
>+skip-if = (os == "android") # Timing issues

I'm not happy about shipping on Android without tests. Either disable the feature on Android or enable the tests.

	Jet Villegas (inactive)
	Comment 70 • 7 years ago

(In reply to Jet Villegas (:jet) from comment #69)
> Comment on attachment 8819102 [details] [diff] [review]
> Enable Tests. r=smaug
> 
> 
> > [test_intersectionobservers.html]
> >-skip-if = true # Track Bug 1320704
> >+skip-if = (os == "android") # Timing issues
> 
> I'm not happy about shipping on Android without tests. Either disable the
> feature on Android or enable the tests.

To be clear, I strongly prefer enabling the tests but can live with a follow-up bug that enables both the feature and tests on Android.

	Tobias Schneider [:tobytailor] Assignee
	Comment 71 • 7 years ago

I just kicked of another try run to see if the timing issues we had on Android in the past are still a concern after latest test fixes landed. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=2752540554df9a496acf3fee2ab584c2658413da.

	Tobias Schneider [:tobytailor] Assignee
	Comment 72 • 7 years ago

Looks like its save now to enable all tests on Android.

	Jet Villegas (inactive)
	Comment 73 • 7 years ago

Comment on attachment 8859343 [details] [diff] [review]
Enable Intersection Observer tests on Android

LGTM, Thx!

	Ryan VanderMeulen [:RyanVM]
	Comment 74 • 7 years ago

FWIW, when dealing with flaky tests, you almost certainly need to retrigger the run multiple times to get a good feel for whether it's truly working better or not. I went ahead and did so and it appears that bug 1313927 is still happening on Android.
https://treeherder.mozilla.org/logviewer.html#?job_id=92516193&repo=try

	Jet Villegas (inactive)
	Comment 75 • 7 years ago

(In reply to [Out of Office Until 24-April] Ryan VanderMeulen [:RyanVM] from comment #74)
> FWIW, when dealing with flaky tests, you almost certainly need to retrigger
> the run multiple times to get a good feel for whether it's truly working
> better or not. I went ahead and did so and it appears that bug 1313927 is
> still happening on Android.
> https://treeherder.mozilla.org/logviewer.html#?job_id=92516193&repo=try

We'll need to disable just that one test on Android due to multi-window flakiness on that platform. That seems to do the trick:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f7bdcdede610a083f6de5a6fb78e387e91d12a5

	Tobias Schneider [:tobytailor] Assignee
	Comment 76 • 7 years ago

Try run with patche from Bug 1313927: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48324feaf319a2f1a3878f64d15d81035bfb975c

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 77 • 7 years ago

Canceled the Try pushes for this bug because they run everything 20 times, causing a huge backlog on Windows 7 and OSX.

	Tobias Schneider [:tobytailor] Assignee
	Comment 78 • 7 years ago

New, single, try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f92377d96988126fe5356911c3133cffb24e1b7

	Jet Villegas (inactive)
	Comment 79 • 7 years ago

dholbert will land the changesets once Try completes and results look good.

	Daniel Holbert [:dholbert]
	Comment 80 • 7 years ago

After looking at this a bit closer, to prepare for landing, I left a bit of feedback on bug 1313927 comment 39 - 40 and in #layout.

Ideally, I'd like for us to structure things so we can land the pref-flip patch as an atomic change (separate from the test changes), so that we can trivially flip the pref again to disable this feature, if we need to.  That is a goal worth striving for, for *all* pref-controlled features -- it's not always possible, but I think it's pretty easy to do here -- I just posted in #layout with some thoughts on how we can accomplish that by tweaking bug 1313927's patch.  I don't think that should hold this up too much -- though it might mean we want one more Try run.

(Inbound seems to be closed for the time being, too, FWIW, so we can't land this for a little while regardless.)

	Pulsebot
	Comment 81 • 7 years ago

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b66b6bb87d11
Enable IntersectionObserver tests. r=jet
https://hg.mozilla.org/integration/mozilla-inbound/rev/32dbe6ca353f
Enable IntersectionObserver. r=jet

	Daniel Holbert [:dholbert]
	Comment 82 • 7 years ago

Patches landed. I took the patches from this Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95acba62d571c397d08bed74cf963d9f700f9e7e

...and I did the following:
 - Adjusted the test patch on this bug to leave the test disabled on android, so that this bug's patches can stand on their own without bug 1313927.
 - Updated bug 1313927 to remove that annotation when it splits the test. (So we end up in the same state, but with each intermediate point building&passing tests)
 - Tested this bug's amended patches on their own, and then with bug 1313927 layered on top:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8b06a3bf9de769e26b26c615ef400cf5f795212
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fba39752893521bb2a0804f9c67f57bd2c5eede
(Ignore the dom mochitest failure there that produced a bunch of orange 7's -- that's unrelated inbound bustage)
 - Added "r=jet" to them (I'm assuming they were substantially similar to the ones on the bug here)
 - Landed!

	Carsten Book [:Tomcat]
	Comment 83 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b66b6bb87d11
https://hg.mozilla.org/mozilla-central/rev/32dbe6ca353f

	Tobias Schneider [:tobytailor] Assignee
	Comment 85 • 7 years ago

Comment on attachment 8864639 [details] [diff] [review]
Patch for beta uplift (Enable tests only)

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: IntersectionObserver API not available. Chrome already ships this feature.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[List of other uplifts needed for the feature/fix]: 1353529, 1313972, 1324135, 1313927

	Ryan VanderMeulen [:RyanVM]
	Comment 90 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/d833c4cac4a7

	Ryan VanderMeulen [:RyanVM]
	Comment 91 • 7 years ago

I had to disable test_intersectionobservers.html due to failures on Beta:
https://treeherder.mozilla.org/logviewer.html#?job_id=96759262&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/4b40534bf2f8

	Tobias Schneider [:tobytailor] Assignee
	Comment 92 • 7 years ago

Can we re-land the updated patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1313972 and re-enable?

	Ryan VanderMeulen [:RyanVM]
	Comment 93 • 7 years ago
bugherder uplift

Ran it through Try and it looks good.
https://hg.mozilla.org/releases/mozilla-beta/rev/e767c39dad3a

	Jet Villegas (inactive)
	Comment 94 • 7 years ago

The Beta patches don't enable IntersectionObserver in FF54, just the tests (which use SpecialPowers to enable the feature for test runs.) The plan is to test IntersectionObserver in Beta via an Experiment.

	Tobias Schneider [:tobytailor] Assignee
	Comment 95 • 7 years ago

Yes, we only uplifted the test enabling part here. Landing the pref-flipping patches will happen via Bug 1362168 if the experiment (Bug 1362418) was successful.

	Eric Shepherd [:sheppy]
	Comment 96 • 7 years ago

The Intersection Observer docs are essentially done; there are a few things I'd like to polish (primarily around diagrams and text to help clarify how things work in places), but for now this is as done as it will be for the next little while. See bug 1386607 for that work.