667664 - Panel content scripts don't work after reloading or changing location

Status: RESOLVED FIXED

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

Description

[Will Bamberg [:wbamberg]]

See: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/275dc226dbb033bc/0bfe727b87be4f65?lnk=gst&q=err_destroyed#0bfe727b87be4f65. 

If a panel displays content from a URL and the location is changed or the page is reloaded, then that panel's content scripts stop working, raising the error:

ERR_DESTROYED, "The page has been destroyed and can no longer be used."

As the group suggests, using an iframe is an effective workaround, but we should consider whether this should be fixed.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

On second thought, this sounds pretty bad, especially if it happens when a page is merely reloaded (document.location changes may not be as common, but pages get reloaded by users quite regularly).  Retriaging.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I hit this the other day. (This actually is even worse than the location changing case, since my example doesn't even change locations!)

In my add-on, I had a panel with three links that can be clicked. When they are clicked, the panel's contentScript sends an event back to the main add-on script for it to do something. I set the links' href attribute to |""| so that they don't actually go anywhere (but they get the default link styling because the href attribute is present).
But because of this bug, clicking the links kills the contentScript completely, so that the custom event never gets emitted.

I got around it by removing the empty href attribute and just giving the links a link-esque styling via CSS.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Handle panel document location change

This patch allows to handle panel location changes by executing the content script on each document loaded into the panel.

Wes: You can avoid <a> links to change location by using <a href="#">link</a>.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 551789 [details] [diff] [review]
Handle panel document location change

This looks good, I don't see anything wrong with it, but I'm seeing a variety of test failures on Mac after applying it.  The failures are inconsistent but are mostly various tests in test-selection.  And they seem to be associated with the added test, since the failures go away when I remove the new test from test-panel.js, even if I don't actually remove the implementation change.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Remove duplicate call to waitUntilDone

That's not the first time I got into such issue: I let duplicate calls to test.waitUntilDone(). That always leads to weird issue in some test after the related one ...

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 565266 [details] [diff] [review]
Remove duplicate call to waitUntilDone

Aha!

How risky do you think this patch is?

Comment 8

[Alexandre Poirot [:ochameau]]

I know it is a common and painfull issue, but this fix may uncover edge cases with:
 - scripts that already contain bugfix around this,
 - content script been executed multiple times that end up breaking stuff.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Ok, then let's land it in the development branch but not cherry-pick it to stabilization.

Comment 10

[Alexandre Poirot [:ochameau]]

Landed in master:
https://github.com/mozilla/addon-sdk/commit/9591dd292243fb05ab890e7f1a0ea22d59dbed2c