1060695 - switch sdk/selection to use a frame script for selection events

Status: RESOLVED INCOMPLETE

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

Description

[Tomislav Jovanovic :zombie]

this module is not too high on the list of priorities (it's not used that much by addons), but it's bad because it doesn't just "fail to work" under e10s, it violently throws upon loading, so any addon that even requires it would probably fail to initialize properly..

at the very least (in the short run), we should guard against that.

Comment 1

[Chris Peterson [:cpeterson]]

This Add-on SDK exception also breaks the "Flip or Rotate Image" addon: bug 987937.

Comment 2

[Allison Naaktgeboren :ally]

Also Bug 1058538 - "DuckDuckGo Plus" add-on

Comment 3

[Edward]

Also Bug 1037198 - "SearchPreview" (Add-on SDK port, not yet in AMO) add-on

Comment 6

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

hey zombie, do you have time for this?  it's Dave's team is supposed to pick up the e10s work according to Joe, so I think we should push this bug to them since it's been broken for so long and anything using jpm & selection module is currently not working..

Comment 7

[Tomislav Jovanovic :zombie]

yeah, this slipped off my radar, will take a look soon.

but i'm not sure i understand how is this related to JPM? does it throw regardless if you use e10s or not? if so, than that's probably a similar-but-different (timing) issue, though likely solvable with the same fix (that's why i'm asking).

Comment 8

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

(In reply to Tomislav Jovanovic [:zombie] from comment #7)
> yeah, this slipped off my radar, will take a look soon.
> 
> but i'm not sure i understand how is this related to JPM? does it throw
> regardless if you use e10s or not? if so, than that's probably a
> similar-but-different (timing) issue, though likely solvable with the same
> fix (that's why i'm asking).

e10s is enabled by default on nightly, jpm only works on nightly, therefore the selection module does not work for jpm..

Comment 9

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

Hey Tom, if you aren't working on this could you please unassign yourself?

Comment 10

[Tomislav Jovanovic :zombie]

sure, sorry about that..

Comment 11

[Tomislav Jovanovic :zombie]

i'm gonna tackle this now..


(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #8)
> e10s is enabled by default on nightly, jpm only works on nightly, therefore
> the selection module does not work for jpm..

right makes sense, i was just being dim..

Comment 12

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1826

this basically adds a selection-events.js frame script, where all the work of tracking windows & attaching selection listeners is done now, while the actual getting/setting of selection still uses CPOWs (with a slight update to the focus-related code). 

all the tests pass in e10s mode, with just a single line change to a helper function.


as discussed in irc last week, i'm still not clear and need to think/consult about:
  1) feasibility of making this sync api work without CPOWs (while not causing too much trouble), or 
  2) if it makes more sense to introduce a new async api here (in which case it's probably not worth trying to do 1).

Comment 13

[Tomislav Jovanovic :zombie]

note: just added a second commit to enable selection tests in an e10s addon, so again, it's probably easier to review the separate commits:

https://github.com/mozilla/addon-sdk/pull/1826/commits

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8546224 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1826

r+ish. There are a couple of questions in there, if there are good reasons then please include them as code comments.

Comment 15

[Tomislav Jovanovic :zombie]

ah, i was just about to ask if you started on this one, or if i could cancel the review?


(In reply to Tomislav Jovanovic [:zombie] from comment #12)
> while the actual getting/setting of selection still uses CPOWs 

as according to new development, this isn't good enough anymore. :\


>   1) feasibility of making this sync api work without CPOWs (while not
> causing too much trouble), 

turns out only one thing about this was "hard": setting the selection as html and then immediately trying to read it as text, or vice versa (not that i know why anybody would do that, but we have a test for it).

Comment 17

[Chris Frost]

+1

I'm encountering this on my own addon (Moo Later). Developed using JPM 1.0.2 and using only the high-level APIs. With the multiprocess shim turned on I see hundreds of CPOWs warnings on the console caused by requiring the selection API. The Addon does keep running though.

let selection = require('sdk/selection');

With the multiprocess shim turned off it throws an error and locks my addon up. They error reported is "TypeError: window is null".

Firefox version 43.0a2 (2015-10-13). Not a problem at all on Firefox 38.* where multiprocess support isn't enabled yet.

Comment 18

[Gabor Krizsanits (INACTIVE)]

This is a high level SDK API and it's not e10s ready. Should we block on this one?

Comment 19

[Andy McKay]

There's 269 add-ons on mxr using sdk/selection.

Comment 20

[u580772]

Comment on attachment 8546224 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1826

>Error retrieving Github pull request diff for https://github.com/mozilla/addon-sdk/pull/1826

Comment 21

[Tomislav Jovanovic :zombie]

42

Comment 22

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562