Closed Bug 642004 Opened 13 years ago Closed 13 years ago

require("context-menu") breaks bfcache making back/forward navigation much slower

Categories

(Add-on SDK Graveyard :: General, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: asqueella, Assigned: adw)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

Bug 578849 added code registering an unload event handler for *every* page ever loaded in the browser <https://github.com/mozilla/addon-sdk/commit/128935631b5b22f305b7cdca0f1ac1de0b3168fa>.

Registering an unload disables bfcache and makes navigating back/forward significantly slower on pages where it is otherwise enabled (e.g. wikipedia).

Of course I'd argue that adding a context menu (that may or may not be invoked on a given page) shouldn't cause any additional code running when a page loads...

This issue is what makes Bugzilla Tweaks slow down the browser on unrelated sites, BTW.
Keywords: perf
The "additional" code runs because the module needs to create a worker for the newly loaded page and destroy the worker when the page unloads.  As an optimization (that should have been obvious to me when I worked on bug 578849), none of that should happen if the menu item has a URLContext that doesn't match the page, yes.

I looked through the page-mod and worker implementations to see how they handle content window unloads -- presumably page-mod also destroys workers when their content windows are unloaded.  But I don't see that that actually happens.  It does, however, add DOMContentLoaded listeners to newly opened matching pages, same as context-menu.
Oh, I should acknowledge another optimization alluded to in comment 0.  The worker for a page could be created when the menu is first invoked on the page, yes.  That's more than an optimization though, since it potentially affects content scripts that expect to be alive beyond the time that the menu is open.  But maybe it's a change we want to make, especially considering we haven't defined the lifetime of context-menu content scripts anywhere.  Likewise, the worker could be destroyed when the menu is closed instead of on page unload.  Same caveat applies there.
I've been reading up on detecting window loads and unloads.  To avoid adding unload listeners to pages, context-menu could probably use the "inner-window-destroyed" nsIObserver notification [1].  In addition to not messing up bfcache, that would mean that context-menu context scripts are alive as long as they're in bfcache.  (And of course it doesn't address the optimizations in comment 1 and 2.)

[1] https://developer.mozilla.org/en/Observer_Notifications
Attached patch patch (obsolete) — Splinter Review
Man, I really suck.  What a horrible problem.  Thank you for pointing it out Nickolay.

This patch does what comment 3 says: uses the inner-window-destroyed notification instead of unload.  This is kind of nice anyway, because it made me realize I can use inner window IDs to uniquely identify windows.  (I had been using URLs to non-uniquely ID them.)  Since window IDs are now unique, I flipped the rows and cols of the WorkerRegistry matrix: it used to be item ID first, window ID second, and now it's window ID first, item ID second.

This means that context-menu workers are alive as long as their pages are in the bfcache.

The patch also beefs up the test by ensuring that context-menu cleans itself up when the loader is unloaded.

Myk, BTW, do you know why page-mod doesn't seem to ever destroy its workers?

I'll work on other optimizations mentioned in this bug and post back here when I file bugs for them.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attachment #521085 - Flags: review?(myk)
Note that if you do have to use unload listeners, registering them on the _browser_, not on the window, will allow bfcache to continue working...
Comment on attachment 521085 [details] [diff] [review]
patch

Welcome users I've screwed over!

Working on a slightly better patch.
Attachment #521085 - Flags: review?(myk)
Attached patch patch 2 (obsolete) — Splinter Review
The previous patch was stupidly storing windows in the WorkerRegistry using a list of { window, key } objects, so looking up a window was O(n).  This patch uses a key => window map instead.  It also does the same for WorkerRegistry items.

See comment 4 for a description of other changes, which this patch keeps intact.
Attachment #521085 - Attachment is obsolete: true
Attachment #521312 - Flags: review?(myk)
Attached patch patch 3Splinter Review
One more, sorry Myk.  There's no need anymore to keep a WorkerRegistry for each browser window.  This patch keeps a single one for the browserManager.
Attachment #521312 - Attachment is obsolete: true
Attachment #521312 - Flags: review?(myk)
Attachment #521342 - Flags: review?(myk)
Comment on attachment 521342 [details] [diff] [review]
patch 3

Nice!
Attachment #521342 - Flags: review?(myk) → review+
https://github.com/mozilla/addon-sdk/commit/bd83f6a6888ade456e026149a85145a8bb781543

I'm working on a patch for an optimization similar to the one mentioned in comment 1.  When I finish I'll file a bug for it and link to it here.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b5
(In reply to comment #10)
> https://github.com/mozilla/addon-sdk/commit/bd83f6a6888ade456e026149a85145a8bb781543
> 
> I'm working on a patch for an optimization similar to the one mentioned in
> comment 1.  When I finish I'll file a bug for it and link to it here.

Is this safe enough for me to release a new version of Bugzilla Tweaks based on this?
I doubt it.
(In reply to comment #12)
> I doubt it.

Can you please ping me about about it when you think it would be the right time?  Thanks!
(In reply to comment #10)
> I'm working on a patch for an optimization similar to the one mentioned in
> comment 1.  When I finish I'll file a bug for it and link to it here.

bug 648434
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: