748677 - Updating the contentURL of a widget or panel breaks content script usage

Status: RESOLVED WONTFIX

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

Description

[Zeke]

User Agent: Mozilla/5.0 (Windows NT 5.2; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

This widget is based on the sample annotator project (https://addons.mozilla.org/en-US/developers/docs/sdk/latest/dev-guide/tutorials/annotator/widget.html). The difference is that the activation status is bound to a tab. So the add-on script will listen on the "activate" event of tabs to update the widget picture (by setting its contentURL property).

The add-on is on the Add-on Builder: 
https://builder.addons.mozilla.org/addon/1049903/latest/


Actual results:

When the widget is clicked, an exception may be thrown sometimes (If it doesn't happen, try to switch tabs and click on the widget again). Here is the error:

Timestamp: 2012-4-25 15:09:58
Error： An exception occurred.
Traceback (most recent call last):
  File "resource://jid0-lmo7pxv7ahsovdliflctlpjw4dk-at-jetpack/api-utils/lib/timer.js", line 38, in notify
    this.callback.apply(null, this.arguments);
  File "resource://jid0-lmo7pxv7ahsovdliflctlpjw4dk-at-jetpack/api-utils/lib/content/worker.js", line 86, in 
    self._emit.apply(self, JSON.parse(args));
  File "resource://jid0-lmo7pxv7ahsovdliflctlpjw4dk-at-jetpack/api-utils/lib/events.js", line 119, in _emit
    return this._emitOnObject.apply(this, args);
  File "resource://jid0-lmo7pxv7ahsovdliflctlpjw4dk-at-jetpack/api-utils/lib/events.js", line 149, in _emitOnObject
    listener.apply(targetObj, params);
  File "resource://jid0-lmo7pxv7ahsovdliflctlpjw4dk-at-jetpack/api-utils/lib/content/worker.js", line 174, in portEmit
    self._addonWorker._onContentScriptEvent.apply(self._addonWorker, arguments);
TypeError: self._addonWorker is null



Expected results:

According to the document(https://addons.mozilla.org/en-US/developers/docs/sdk/latest/packages/addon-kit/widget.html#contentURL_10), setting the contentURL is a valid operation and should not breake the content script.

Comment 1

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

Confirming that clicking the widget eventually triggers the self._addonWorker is null error.

Comment 2

[John Nagle]

I've received a similar error message with an page-mod add-on of mine.  This may or may not be related.  The problem is intermittent.  When this occurs, the add-on does not run for the page involved. 

Timestamp: 5/7/2012 11:56:52 AM
Error: An exception occurred.
Traceback (most recent call last):
  File "resource://551f2920-3c19-11e1-b86c-0800200c9a66-at-jetpack/api-utils/lib/content/worker.js", line 174, in portEmit
    self._addonWorker._onContentScriptEvent.apply(self._addonWorker, arguments);
TypeError: self._addonWorker is null

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

I see the same result with panels:

Error: Couldn't find the worker to receive this message. The script may not be initialized yet, or may already have been unloaded.
resource://jid1-k0azbtc0pugmbg-at-jetpack/addon-sdk/lib/sdk/content/worker.js 624
Traceback (most recent call last):
  File "resource://jid1-k0azbtc0pugmbg-at-jetpack/ubilite/lib/main.js", line 43, in .onTrack/<
    value: query
  File "resource://jid1-k0azbtc0pugmbg-at-jetpack/addon-sdk/lib/sdk/content/worker.js", line 406, in Worker<.postMessage
    processMessage.apply(this, args);
  File "resource://jid1-k0azbtc0pugmbg-at-jetpack/addon-sdk/lib/sdk/content/worker.js", line 624, in processMessage
    throw new Error(ERR_DESTROYED);

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Erik, been digging into this -- on `contentURL`, should the worker be destroyed and recreated/reinitialized? Looks like the symbiont/worker is destroyed, but persists just without a hook to the widget/panel. I'd imagine since the contentURL is being changed, the worker should be recreated/reinjected from scratch, but that may break persistence when port events are not used. Let's talk about this sometime when you're around?

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

I think that the content script should be recreated, let's see what Irakli says.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I think most important issues here is that in order to handle click events on widgets we require user to create a content-script, just to know what was clicked and with which mouse button. That's makes API terrible and and wastes a lot of resources. In addition provides surface for bugs like this one :(

What I think we should do is:

1. Pass some event data to the click handler on the widget that will make use of content-scripts unnecessary and rewrite our examples to use that data instead. For the start I'd pass object with a following properties:

  {
    type: "click",

    // Info about event.target element.
    target: {
      id: "foo", // element id if it has one.
      nodeName: "img",
      namespaceURI: "http://www.w3.org/1999/xhtml",
      classList: ["icon", "button"],
    },

    // Details for following attributes:
    // https://developer.mozilla.org/en-US/docs/DOM/Mozilla_event_reference/click
    button: int,
    detail: int,
    mozPressure: float,
    ctrlKey: boolean,
    shiftKey: boolean,
    altKey: boolean,
    metaKey: boolean,
    screenX: int,
    screenY: int,
    clientX: int,
    clientY: int,
  }

We should do the same for other widget events like mouseover and mouseout. I think we should also add dbclick event to this list of events we expose.

2. As of content scripts re-execution on page changes, I think we should fix that too, or consider deprecating content scripts for widgets entirely in favor of #1.


Jeff, do you think deprecation of content-scripts for widgets is reasonable if we provide #1 ? If that's reasonable move for the project I'd very much favor that.

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Oh btw, if we do end up deprecating content-scripts I would also expose "message" events on the widget content window where `event.source === window`. Where event
data looks as follows:

{
  type: "message",
  data: { /*...*/ },
  origin: string,
  lastEventId: int
}

This way widget still could do very low level stuff and communicate with add-on host via postMessage.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

+1 on #1, also a right-click event in addition to the others

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

My only concern is that I'd like to eventually deprecate widget entirely in favour of simpler apis, so I don't know how much work we want to put into it. But sure, #1 sounds fine to me.

Comment 11

[Marc Chevrier]

#1 sounds good but middle-click must also be handled as well.

Comment 12

[Dave Townsend [:mossop]]

When Australis lands widget will be deprecated so we won't be working on this.