607675 - In Firefox, YAHOO.util.Event.addListener/on events no longer exist after a user clicks back

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Frédéric Buclin]

Only affects 4.0 and 4.1. This is working fine in 3.6.3:

STR:
0. I use Firefox 3.6.11 on Linux!
1. View an open bug (show_bug.cgi): you note that when you select RESOLVED, the resolution select menu appears, as expected.
2. Visit any other page, e.g. view an attachment attached to the bug.
3. Click the Back button of your web browser to come back to the bug report: you note that when you select RESOLVED, the resolution select menu no longer appears, and if you click "Save Changes" anyway, you get the following error:

 A valid resolution is required to mark bugs as RESOLVED

Marking as blocker, because I very often view and download an attachment which I need to commit, and then come back to the bug to paste the commit message and close the bug. But this use case fails due to this bug.

Comment 1

[Frédéric Buclin]

WFM with Opera 10.63 and Google Chrome 7.0, but fails with Fx 3.6.11 and Fx 4.0b8pre. So probably has something to do with our Gecko-only hacks.

Comment 2

[Guy Pyrzak]

I can't reproduce this on 4.0 or 3.6 on the mac.

Comment 3

[Frédéric Buclin]

I can reproduce on both Linux and Windows 7.

Comment 4

[Max Kanat-Alexander]

I cannot reproduce with Firefox 3.5.12 on Linux, on bugzilla-4.0-branch.

Comment 5

[Frédéric Buclin]

You are still using Fx 3.5?? Wow! :)

OK, unmarking the bug as blocker as I seem to be the only one who can reproduce (on two different OSes!).

Comment 6

[:glob ✱]

i can reproduce this with fx 3.6.12 on xp.

Error: J is null
Source File: https://landfill.bugzilla.org/bugzilla-4.0-branch/js/yui/yahoo-dom-event/yahoo-dom-event.js?1288573330
Line: 11

Comment 7

[:glob ✱]

note: the JS error is generated when i follow the attachment details link, not when i navigate back to the bug page.

Comment 8

[:glob ✱]

however the first visit to attachment details doesn't generate an error.  only subsequent visits.

Comment 9

[Max Kanat-Alexander]

Okay. Replacing yahoo-dom-event.js with the -debug or the normal version from a YUI tarball should help make that error clearer.

Comment 10

[:glob ✱]

Error: unloadListeners is null
Source File: https://landfill.bugzilla.org/glob_4/js/yui/yahoo-dom-event/yahoo-dom-event.js?1288596488
Line: 4778

Comment 11

[Teemu Mannermaa (:wicked)]

I can reproduce this in 4.0 and trunk with Firefox 3.6.12 (Win7 x64). Looks like using Back-button (doesn't matter where you go) breaks automation of the status/resolution/duplicate# text boxes until the page is reloaded.

This seems to affect also CC List and date picker controls. I'd guess any and all that use some kind of yui automation(?) to show/hide controls.

(In reply to comment #6)
> Error: J is null

For me, this error appears always when the show_bug page is loaded but not when Back-button is pressed so it might not be relevant. There's also tons of "anonymous function does not always return value" warnings from yui code. :(

Comment 12

[Frédéric Buclin]

Thanks for the testing. I'm happy to see it's not just me. :)

Comment 13

[Teemu Mannermaa (:wicked)]

I investigated this a bit. This seems to be related to bfcache in Firefox. Disabling that fixes the problem. It's like Firefox removes any and all yui Javascript change listeners from a page retrieved from the cache. I wonder if that's is on purpose or if we do something that breaks bfcache JS state restore. Although, simple comment expand/collapse JS still works just fine.

I also tested IE just in case and it's not affected by this bug.

Comment 14

[Guy Pyrzak]

we removed the onpageunload listener for firefox because it slowed down page loads when users click back. So it looks like this is a ramification of that. However, this change is also deployed on BMO so we should see the same error on here.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> we removed the onpageunload listener for firefox because it slowed down page
> loads when users click back.

  Actually, as I recall, we now simply move the onunload listener to onpagehide.

Comment 16

[Teemu Mannermaa (:wicked)]

Exactly, and it seems that's causing this bug since reverting patch in bug 577574 fixes this bug (but most likely regresses that bug). Do we know what YAHOO.util.Event._unload does and why it's needed? Or more importantly, how can we revert it's changed when page comes back to live (maybe for onpageshow event)?

Comment 17

[Max Kanat-Alexander]

Well, it is necessary--the bug described is fixed by the change. You might be right that we also need to move an onload into onpageshow, though, too. pyrzak (or others), any thoughts on that?

Comment 18

[Guy Pyrzak]

Personally I think we're doing a lot of work here because of issues with YUI. Can we try to talk to the YUI folks to get this issue resolved? 

To give a full description. We removed onunload because it breaks bfcache for firefox, which causes the back button to be very slow. We put it back on to onpagehide because without the unload it caused the autocompletes to not fix themselves before leaving the page (autocompletes basically break themselves until the users leaves the page). 

I am still not able to reproduce this bug, and I'd like to be able to do that reliably before saying what the cause is.

Comment 19

[Guy Pyrzak]

Ok, I was able to reproduce with a fresh firefox profile with the same bug (./firefox -P). I'm going to see what we can do to track down the actual cause.

Comment 20

[Max Kanat-Alexander]

(In reply to comment #18)
> Personally I think we're doing a lot of work here because of issues with YUI.
> Can we try to talk to the YUI folks to get this issue resolved? 

  Yeah, please do. dwm: If you can help us at all with getting in touch with folks and actually getting this resolved, please let pyrzak know.

> I am still not able to reproduce this bug, and I'd like to be able to do that
> reliably before saying what the cause is.

  It only happens on Firefox 3.6.x and 4.0 betas, not on any other browser or any earlier version of Firefox.

Comment 21

[David Marshall]

I'll let Petey, our YUI guru, know about this.  Have you filed a bug with YUI or found a similar one?  It would probably be easier to get this addressed if there's a bug to point out.

Comment 22

[petey31]

This seems to be related to the bug logged at http://yuilibrary.com/projects/yui2/ticket/2091763.

Can this bug be demonstrated with a page simpler than a complete show_bug page? There's an awful lot of javascript in there to wade through. With a simpler demonstration of the problem it will be easier to get a fix.

Comment 23

[Guy Pyrzak]

it is related to that trouble ticket. 

Basically YUI uses unload as an even, this breaks firefox's bfcache, which caused bugzilla to be very slow for some firefox users. We removed the unload, however, that autocompletes to not work because autocompletes do some fun stuff when the unload is called. 

I can make a page that will reproduce this if that helps but in the end the core bug is caused by using onUnload event in YUI. All the other bugs stem from dealing with that.

Comment 24

[Frédéric Buclin]

Any traction?

Comment 25

[Frédéric Buclin]

This is our last blocker for Bugzilla 4.0. Could someone who knows YUI well enough please fix this bug?

Comment 26

[Guy Pyrzak]

Attachment: v1

This patch fixes the problem. All it does is move the onchange event from being called by YUI to being on the actual select.

As far as I can tell the bug is caused by a bunch of the other stuff we have done. Basically this....

YUI tries to clean up after itself by removing all the events it has added to the page on page unload, isn't that nice of it? Usually this is onpageunload, which breaks bfcache, so we moved it to onpagehide, how smart of us. Great so now YUI still gets to clean up after itself and Firefox caches the page in the state that it was left in after YUI cleans up after itself.

But since we've got BFCache on Firefox when the user hits back the page is in the same state as when YUI cleaned up after itself, all the events on any element of the page is now gone!

The proper fix is to disable the clean up for firefox, or to call all the page unloads but not call the unload listener stuff. We also need to make sure we don't mess this up for IE since they need this to fix a memory leak issue. Checkout Event.js 1809 in version 2.8.1r2 for details on the YUI.

However, since that might take a bit more effort/thought, for now i just fixed the status stuff. However anything that is a YAHOO.Event.addListener might be broken by these fixes. 

LpSolit/Mkanat, let me know if you guys prefer for me to mess with the other option which requires more messing with YUI, which I am hesitant to do, since that's what got us into this hole.

Comment 27

[Guy Pyrzak]

(In reply to comment #26)
OOps i meant, anything that uses YAHOO.Event.addListener on an element might still be broken. This fix doesn't break anything in particular, it just doesn't fix everything that could be broken

Comment 28

[Frédéric Buclin]

Isn't there a way to discuss about the "right fix" (whatever it is) with Petey, and have it included in YUI itself? Is it easy to create a reduced testcase for the YUI team, so that they can more easily track our problem?

Comment 29

[Guy Pyrzak]

an initial search shows we mostly use the event addListener and on calls in places where it shouldn't be a problem. However, just to make sure below is a list of places that might be affected...

./extensions/Voting/template/en/default/hook/admin/products/edit-common-rows.html.tmpl:        YAHOO.util.Event.addListener('allows_unconfirmed', 'change',
./js/field.js:    YAHOO.util.Event.addListener(document.body, 'click', 
./js/field.js:    YAHOO.util.Event.addListener(document.body, 'keydown', calendar.bz_escCal);
./js/field.js:    YAHOO.util.Event.addListener(action, 'click', showEditableField,
./js/field.js:        YAHOO.util.Event.addListener(window, 'load', checkForChangedFieldValues,
./js/field.js:    YAHOO.util.Event.addListener( window, 'load', checkForChangedFieldValues,
./js/field.js:        YAHOO.util.Event.addListener( field_id_list[i],'change', showEditableField,
./js/field.js:        YAHOO.util.Event.addListener( field_id_list[i],'change',showEditableField,
./js/field.js:        YAHOO.util.Event.addListener( field_id_list[i],'change', setDefaultCheckbox,
./js/field.js:        YAHOO.util.Event.addListener( field_id_list[i],'change',setDefaultCheckbox,
./js/field.js:    YAHOO.util.Event.addListener( 'set_default_' + field_id,'change', boldOnChange,
./js/field.js:    YAHOO.util.Event.addListener( window,'load', checkForChangedFieldValues,
./js/field.js:    YAHOO.util.Event.addListener( window, 'load', boldOnChange,
./js/field.js:    YAHOO.util.Event.addListener(controller, 'change', 
./js/field.js:    YAHOO.util.Event.addListener(controller_field, 'change',
./template/en/default/list/edit-multiple.html.tmpl:    YAHOO.util.Event.addListener('bug_status', "change", showHideStatusItems, '[% "is_duplicate" IF bug.dup_id %]');
./js/bug.js:        YAHOO.util.Event.on(data.summary_field, 'keyup', this.doUpdateTable,
./template/en/default/account/auth/login-small.html.tmpl:          YAHOO.util.Event.on(window, 'load', function () {
./template/en/default/bug/field-events.js.tmpl:  YAHOO.util.Event.on('product', 'change', setClassification);

so it looks like edit multiple, the create page, edit products, and a few things in field.js. Let me know if you guys want me to go down the path of trying to fix the way load/unload is handled. It just seems REALLy messy to be copying the YUI code into our own and syncing those changes. Preferably YUI would at least modularize the 2 parts of their unload code so we can call the unload events without calling the remove listener part of the code.

Comment 30

[Guy Pyrzak]

 (In reply to comment #28)
> Isn't there a way to discuss about the "right fix" (whatever it is) with Petey,
> and have it included in YUI itself? Is it easy to create a reduced testcase for
> the YUI team, so that they can more easily track our problem?

The problem is we tried to maintain BFCache for Firefox users, YUI breaks it, then we did a bunch of stuff we shouldn't have to maintain it. The core fix is for YUI to stop using onpageunload, but they have already said they will not do that since that's a HUGE refactor for them. The other option for us is to not worry about maintaining BFCache for Firefox.

Comment 31

[Frédéric Buclin]

(In reply to comment #30)
> The core fix is
> for YUI to stop using onpageunload, but they have already said they will not do
> that since that's a HUGE refactor for them.

We cannot break bfcache in Firefox. This makes going back way too slow. I don't understand the YUI team about not wanting to refactor their code when they know the current one breaks the 2nd most used browser in the world. At least a not perfect workaround would be better than the current situation. YUI should be usable with minimum effort. If we are doing something fondamentally wrong on our side, we should fix it too. But I don't know YUI well enough to comment further.

Comment 32

[Guy Pyrzak]

Attachment: Example without all the mess

So here is an example without all the other bugzilla stuff. One option is to move the autocomplete back fix to our own code to handle it instead of moving the whole unload to onpagehide. This will fix the bug that was filed for back not autofilling but I'm less sure about what we'd miss. We could also just copy and paste the code that is in the unload into our own listener for onpagehide.

Comment 33

[Guy Pyrzak]

Attachment: v2

Based on the fix that Petey supplied i made this patch. Let me know if we want to apply the same style guidelines to this chunk of JS or not. Basically it overrides addListener to use pagehide instead of unload. Thanks to Petey for figuring this one out.

Comment 34

[Frédéric Buclin]

Comment on attachment 493838 [details] [diff] [review]
v2

This fixes the problem for me, so r=LpSolit based on my testing. But I'm totally unable to review the code itself as I know mosty nothing about YUI. I will let mkanat review the code itself, and if he is fine with it, we can commit the patch.

Comment 35

[Max Kanat-Alexander]

Comment on attachment 493838 [details] [diff] [review]
v2

Okay, the added code looks fine, but is this addListener fixup the only thing that happens during onunload? Because now we're removing the entire onunload handler, but only adding a fix for addListener.

Comment 36

[Max Kanat-Alexander]

Comment on attachment 493838 [details] [diff] [review]
v2

Okay, pyrzak explained this to me on IM. Here's what this patch does and why it works:

This hacks addListener to add a "pagehide" event every time somebody adds an "unload" event. (Of course, it only does this for browsers that support pagehide.) As a result, we don't need to move the "unload" event to "pagehide" anymore. So actually, this should totally solve all of our unload-related issues from here on out, unless somebody tries to add an event to window.onunload directly without calling addListener.

Comment 37

[Max Kanat-Alexander]

Beautiful, thank you pyrzak. :-)

Comment 38

[Guy Pyrzak]

it was mostly petey. w00t 4.0!

Comment 39

[Guy Pyrzak]

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/  
modified template/en/default/global/header.html.tmpl
Committed revision 7488. 

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/global/header.html.tmpl
Committed revision 7610.