Enable IntersectionObserver

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: smaug, Assigned: tschneider)

Tracking

({dev-doc-complete})

Trunk
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 disabled, firefox55 fixed)

Details

Attachments

(5 attachments, 7 obsolete attachments)

1.56 KB, patch
Details | Diff | Splinter Review
797 bytes, patch
bugs
: review+
Details | Diff | Splinter Review
48 bytes, text/plain
Details
746 bytes, patch
bugs
: review+
Details | Diff | Splinter Review
2.52 KB, patch
Details | Diff | Splinter Review
Reporter

Description

3 years ago
Once the stability issues have been fixed, the API should be enabled again.
Version: 50 Branch → Trunk
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 ]
Priority: -- → P3
Depends on: 1322717
Assignee

Comment 2

3 years ago
Posted patch Enable IntersectionObserver (obsolete) — Splinter Review
Once Bug 1322717 is landed, this patch flips the pref to enable the IntersectionObserver API.
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.
Flags: needinfo?(tschneider)
Assignee

Comment 4

3 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.
Flags: needinfo?(tschneider)
Good to hear! And yes, the Windows population is *much* larger than Mac and Linux on all channels.
Assignee

Updated

3 years ago
Keywords: checkin-needed
Landing this with an implicit r=smaug given the belief that the crashes are indeed fixed now.
Assignee: nobody → tschneider

Comment 7

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ffd7d13eb96
Enable IntersectionObserver. r=smaug
Keywords: checkin-needed
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)
Flags: needinfo?(tschneider)
Reporter

Comment 9

3 years ago
r=smaug to add IntersectionObserver to the test_interfaces.html list.
Reporter

Comment 10

3 years ago
but but, doesn't one need to also enable the tests again too, the one which bug 1320704 disabled.
Posted patch Enable Tests (obsolete) — Splinter Review
Flags: needinfo?(tschneider)
Assignee

Updated

3 years ago
Attachment #8817931 - Flags: review?(bugs)
Reporter

Updated

3 years ago
Attachment #8817931 - Flags: review?(bugs) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
backed out for test failurs like https://treeherder.mozilla.org/logviewer.html#?job_id=40567662&repo=mozilla-inbound
Flags: needinfo?(tschneider)

Comment 14

3 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
Keep disabled on android.
Attachment #8817931 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3ae78eda5a0c
https://hg.mozilla.org/mozilla-central/rev/016b87fe9145
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

3 years ago
Depends on: 1324209
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
Status: RESOLVED → REOPENED
Flags: needinfo?(tschneider)
Resolution: FIXED → ---
Assignee

Updated

2 years ago
Depends on: 1325103
Flags: needinfo?(tschneider)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e3ce9502a04
https://hg.mozilla.org/mozilla-central/rev/394aea8026a3
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → FIXED
Depends on: 1326194
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.
Flags: needinfo?(tschneider)
Flags: needinfo?(ryanvm)
Flags: needinfo?(bugs)
RyanVM can you please back this out until I have the crashes fixed?
Flags: needinfo?(tschneider)
Status: REOPENED → ASSIGNED
Flags: needinfo?(ryanvm)
Target Milestone: mozilla53 → ---
Assignee

Updated

2 years ago
Keywords: checkin-needed
Was comment 23 ever addressed regarding a test plan? FWIW, I'm inclined to agree with Nick's assessment here.
Keywords: checkin-needed
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.
Assignee

Updated

2 years ago
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=67922908&repo=mozilla-inbound
Flags: needinfo?(tschneider)

Comment 30

2 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
Posted patch Enable IntersectionObserver (obsolete) — Splinter Review
Rebased patch.
Attachment #8817663 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
The real one.
Attachment #8825913 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd23e1fc730b
https://hg.mozilla.org/mozilla-central/rev/31511bed6415
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

2 years ago
Depends on: 1332939
Ryan, please back out again due to newly reported crashes.
Flags: needinfo?(ryanvm)
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
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---
Tobias: Please set me to r? before landing this one again. Thx!
Assignee: tschneider → bugs
Flags: needinfo?(bugs)
Assignee: bugs → tschneider
Flags: needinfo?(bugs)
Reporter

Comment 39

2 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.
Right now we've 1087 crashes in nightly with build-id 20170122030212.
Assignee

Updated

2 years ago
Depends on: 1333580
:truber is working on adding support to existing fuzzers to get some fuzz coverage.
(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.
(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.
Posted file Test Plan
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
Assignee

Updated

2 years ago
Depends on: 1337936
Reporter

Updated

2 years ago
Depends on: 1338886
Assignee

Updated

2 years ago
Depends on: 1351500
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!
Attachment #8825915 - Flags: review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 49

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/718fb66559f7
Enable IntersectionObserver and tests. r=smaug, r=jet
Keywords: checkin-needed
Depends on: 1313972

Comment 50

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/718fb66559f7
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
With Bug 1313972 being fixed, let's re-land this.
Assignee

Updated

2 years ago
Keywords: checkin-needed
Can we verify that on Try first?
Keywords: checkin-needed
Lovely, thanks :). Don't hesitate to include recent Try links in the future when requesting checkin as well.
Keywords: checkin-needed

Comment 57

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/83041db77fcf
Enable IntersectionObserver and tests. r=smaug, r=jet
Keywords: checkin-needed
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
Flags: needinfo?(tschneider)
Mh, did that Try task run with the patch from Bug 1313972?
Flags: needinfo?(tschneider)

Comment 60

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/25b64d100bda
Enable IntersectionObserver and tests. r=smaug, r=jet
Ah, Groundhog Day is late this year, apparently.

Backed out again in https://hg.mozilla.org/integration/autoland/rev/2a494e1f39e7
(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?
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.
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.
(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.
Was that the first try run after the patches landed? I see a couple other timeouts, which might be just coincidence tho.
Latest try run finally looking good after Bug 1354163, 1324135 and 1313972 being fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b443f737b08d934457b900e83707dd021c306cf
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.
Flags: needinfo?(tschneider)
(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.
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.
Flags: needinfo?(tschneider)
Looks like its save now to enable all tests on Android.
Assignee

Updated

2 years ago
Attachment #8859343 - Flags: review?(bugs)
Comment on attachment 8859343 [details] [diff] [review]
Enable Intersection Observer tests on Android

LGTM, Thx!
Attachment #8859343 - Flags: review?(bugs) → review+
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
Blocks: 1131937
(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
Canceled the Try pushes for this bug because they run everything 20 times, causing a huge backlog on Windows 7 and OSX.
dholbert will land the changesets once Try completes and results look good.
Flags: needinfo?(dholbert)
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.)
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!
Flags: needinfo?(dholbert)

Comment 83

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b66b6bb87d11
https://hg.mozilla.org/mozilla-central/rev/32dbe6ca353f
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Updated

2 years ago
Blocks: 1362168
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
Attachment #8864639 - Flags: approval-mozilla-beta?
Attachment #8864639 - Attachment is obsolete: true
Attachment #8864639 - Flags: approval-mozilla-beta?
Attachment #8864645 - Attachment is obsolete: true
Attachment #8864655 - Attachment is obsolete: true
Can we re-land the updated patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1313972 and re-enable?
Flags: needinfo?(tschneider)
Assignee

Updated

2 years ago
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
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.
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.
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.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.