852279 - Bug pages no longer added to bfcache due to unload listener on Window

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

+++ This bug was initially created as a clone of Bug #453459 +++

After the most recent Bugzilla upgrade, none of the bugzilla pages are added to bfcache, as far as I can see.

STEPS TO REPRODUCE:
1)  Load https://bugzilla.mozilla.org/show_bug.cgi?id=851378
2)  Load https://bugzilla.mozilla.org/attachment.cgi?id=725567
3)  Click the "Back" button in your browser

EXPECTED RESULTS: speedy back

ACTUAL RESULTS: slow back

ADDITIONAL INFORMATION:  The issue seems to be that there is an unload listener attached to the Window from https://bugzilla.mozilla.org/js/yui/yahoo-dom-event/yahoo-dom-event.js?1361965969 line 11.  Any time that happens, we have to turn off bfcache.

This is making using Bugzilla pretty miserable, especially when following dependency chains or trying to do reviews.

Comment 1

[Frédéric Buclin]

Maybe a regression due to bug 607675? See bug 607675 comment 13 and following. Though the bug is targetted 4.0, so we should have noticed it before the upgrade.

Comment 2

[Frédéric Buclin]

There are two changes which could explain why 4.2 is broken and 4.0 was not:

- Displaying a Custom Field Value Based on Multiple Values of Another Field
- YUI has been upgraded to 2.9.0

I wonder if the first one has some code which could badly interact with bfcache, or if YUI 2.9.0 has more listeners compared to 2.8 which could explain why bfache is broken. No idea how to check that.

Comment 3

[Frédéric Buclin]

wicked: any idea?

Comment 4

[Frédéric Buclin]

As bmo uses YUI3 for some of its customizations, could it be that YUI3 is responsible for this problem?

Comment 5

[[e-mail]]

Attachment: Stacks

Assuming I set my breakpoint correctly, I found the following two stacks which appeared to set unload listeners.

Comment 6

[:glob ✱]

with help from bz, here's what firefox is reporting as the unload listener:

> function (){e.Event._unload()}

the code we're currently running to remove the listener is:

> window.removeEventListener("unload", YAHOO.util.Event._unload, (undefined))

this isn't working because the listener is an anonymous function wrapping _unload().

however yui does:

> window.addEventListener("unload", EU._unload, (undefined))

so it's a bit of a mystery right now as to where the anon function is coming from.

Comment 7

[:glob ✱]

ah, it's yui3's unload listener.

yui3 was added to all pages in as part of the dashboard development, however it's only used on show_bug for the product and component description toggling (bug 837878).

until we can figure out how to correctly remove yui3's unload listener, we should only load yui3 on pages which need it (enter_bug pages and the dashboards).

Comment 8

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #7)
> until we can figure out how to correctly remove yui3's unload listener, we
> should only load yui3 on pages which need it (enter_bug pages and the
> dashboards).

.. and convert the description toggler to yui2

Comment 9

[David Lawrence [:dkl]]

Attachment: Patch to only use yui3 base on pages that need it (v1)

Comment 10

[:glob ✱]

Comment on attachment 733917 [details] [diff] [review]
Patch to only use yui3 base on pages that need it (v1)

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

r=glob, with the following to be fixed on commit.

::: extensions/BMO/template/en/default/hook/bug/edit-after_importance.html.tmpl
@@ +16,5 @@
> +    if (prod_desc) {
> +      var field_container = Dom.get('field_container_product');
> +      var toggle_container = document.createElement('span');
> +      Dom.setAttribute(toggle_container, 'id', 'toggle_prod_desc');
> +      toggle_container.appendChild(document.createTextNode('('));

you need to use ' (' to keep the separation for not-logged-in views

@@ +47,5 @@
> +    if (comp_desc) {
> +      var field_container = Dom.get('field_container_component');
> +      var toggle_container = document.createElement('span');
> +      Dom.setAttribute(toggle_container, 'id', 'toggle_comp_desc');
> +      toggle_container.appendChild(document.createTextNode('('));

' (' here too

@@ +61,5 @@
> +      Dom.addClass(desc_container, 'bz_default_hidden');
> +      desc_container.innerHTML = comp_desc;
> +      field_container.appendChild(desc_container);
> +      Event.addListener(toggle_link, 'click', function () {
> +        console.log('clicked');

remove debugging code

Comment 11

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bmo/4.2             
modified extensions/BMO/template/en/default/global/choose-product.html.tmpl
modified extensions/BMO/template/en/default/global/header.html.tmpl                                                                               modified extensions/BMO/template/en/default/hook/bug/edit-after_importance.html.tmpl
modified extensions/GuidedBugEntry/template/en/default/guided/guided.html.tmpl
modified extensions/MyDashboard/template/en/default/pages/mydashboard.html.tmpl                    
modified extensions/ProdCompSearch/template/en/default/pages/prodcompsearch.html.tmpl                                                                       modified extensions/ProductDashboard/template/en/default/pages/productdashboard.html.tmpl               
modified template/en/default/global/header.html.tmpl
Committed revision 8726.