787236 - Error when removing Reader:FaviconReturn observer on Reader uninit()

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P1)

Description

[Lucas Rocha (:lucasr)]

Attachment: Correctly remove "pagehide" listener when exiting Reader

We call bind() in the listener function which means the actual listener is not the named function. This patch fixes this.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 657061 [details] [diff] [review]
Correctly remove "pagehide" listener when exiting Reader

Does this mean this ocde is wrong too?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2684

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I assume this code is not affected becuase there is no "bind" ?
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4492

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Comment on attachment 657061 [details] [diff] [review]
> Correctly remove "pagehide" listener when exiting Reader
> 
> Does this mean this ocde is wrong too?
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#2684

Yep, this is broken, the listener is never removed.

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> I assume this code is not affected becuase there is no "bind" ?
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#4492

Correct.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Correctly remove "pagehide" listener for ErrorPageEventHandler

Comment 6

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49ad41173c4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5489c99c67d6

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49ad41173c4f
https://hg.mozilla.org/mozilla-central/rev/5489c99c67d6

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 657061 [details] [diff] [review]
Correctly remove "pagehide" listener when exiting Reader

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Small regression from bug 778582.
User impact if declined: Tons of errors in logcat and potential instability due to dangling listeners.
Testing completed (on m-c, etc.): Landed in Nightly, no issues found.
Risk to taking this patch (and alternatives if risky): Low, trivial patch.
String or UUID changes made by this patch: None

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 657245 [details] [diff] [review]
Correctly remove "pagehide" listener for ErrorPageEventHandler

[Approval Request Comment]
User impact if declined: Dangling listeners and potential instability caused by dangling listeners on error pages (e.g. about:certerror)
Testing completed (on m-c, etc.): Landed in Nightly, no issues found.
Risk to taking this patch (and alternatives if risky): Low, trivial patch.
String or UUID changes made by this patch: None

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 657061 [details] [diff] [review]
Correctly remove "pagehide" listener when exiting Reader

[Triage Comment]
Low risk fix in support of Reader Mode stability. While I don't believe we know what the user impact would really be if declined, we'll approve since we think we can manage the risk at this point.

Comment 11

[Lucas Rocha (:lucasr)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/d575f4b98a7a
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ce657b96136

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/375ae9107d45
https://hg.mozilla.org/releases/mozilla-beta/rev/44a038b9e733

Comment 12

[Cristian Nicolae (:xti)]

Code changes were applied on the latest Nightly, Aurora and Beta builds. Closing bug as verified fixed.

--
Device: Galaxy Note
OS: Android 4.0.4