1377734 - browser.tabs.sendMessage() triggers instantiation of lazy tabs

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[u462496]

browser.tabs.sendMessage() accesses browser.messageManager which triggers early lazy tab instantiation:

[bug 1345098] Lazy browser prematurely inserted via 'messageManager' property access:
getter@chrome://browser/content/tabbrowser.xml:2241:45
getMessageManagerForRecipient@resource://gre/modules/ExtensionParent.jsm:232:7
receiveMessage@resource://gre/modules/ExtensionParent.jsm:198:22
_callHandlers/responses<@resource://gre/modules/MessageChannel.jsm:591:16
_callHandlers@resource://gre/modules/MessageChannel.jsm:589:21
_handleMessage/deferred.promise<@resource://gre/modules/MessageChannel.jsm:649:7
_handleMessage@resource://gre/modules/MessageChannel.jsm:646:24
receiveMessage@resource://gre/modules/MessageChannel.jsm:163:5

Presumably other message APIs may do the same thing.

I don't know whether this should be handled internally, or if it should be up to the addon developer.  In the case of the latter however, we would need something like bug 1377733.

Ideally I think this should be attacked from both sides.

Comment 1

[Andrew Swan [:aswan]]

Kevin, I don't understand the last remark about "from both sides".  Its not clear to me what behavior for sendMessage() would make sense other than instantiating the tab (queue the messgae indefinitely somewhere?)  I agree that it should be possible for extension authors to avoid unnecessarily instantiating lazy tabs, but as you point out that's covered by a separate bug.  I'm inclined to close this unless there's something else you think we can reasonably do?

Comment 2

[u462496]

(In reply to Andrew Swan [:aswan] from comment #1)
> Kevin, I don't understand the last remark about "from both sides".  Its not
> clear to me what behavior for sendMessage() would make sense other than
> instantiating the tab (queue the messgae indefinitely somewhere?)  I agree
> that it should be possible for extension authors to avoid unnecessarily
> instantiating lazy tabs, but as you point out that's covered by a separate
> bug.  I'm inclined to close this unless there's something else you think we
> can reasonably do?

I wouldn't be the judge on what is "reasonable", but simply disallowing sendMessage on lazy tabs comes to mind.  I don't know how feasible queuing messages would be, but sounds like it could be involved.  It seems like we have been making pains to prevent addons from indiscriminately instantiating all tabs which could very easily happen if an addon wishes to send a message to all tabs, and that instantiation of a lazy tab other than the user selecting it should be proactive.  I would gamble that most addon devs are unaware of lazy tabs and special sensibilities involved with them, especially the undesirable UX if a large number of lazy tabs are suddenly instantiated all at once.

Comment 3

[Andrew Swan [:aswan]]

I think you make a good argument for not instantiating tabs by default but that we should give extension authors a way to wake up a tab and send a message to it.  We could do it with an option to sendMessage() but ideally it would be independent of sendMessage() (eg calling update() with the lazy property set to false or something).
I haven't followed all the other bugs related to lazy tabs, is that adequately covered by existing bugs or do we need others in addition to this one?

Comment 4

[u462496]

(In reply to Andrew Swan [:aswan] from comment #3)
> I think you make a good argument for not instantiating tabs by default but
> that we should give extension authors a way to wake up a tab and send a
> message to it.  We could do it with an option to sendMessage() but ideally
> it would be independent of sendMessage() (eg calling update() with the lazy
> property set to false or something).
> I haven't followed all the other bugs related to lazy tabs, is that
> adequately covered by existing bugs or do we need others in addition to this
> one?

We have covered a lot of cases.  In some edge cases we have been able to train addons to not do actions which would instantiate, but this has only been where instantiating properties have been directly accessed by overlay addons which will expire soon anyway.  In other cases we have been able to add traps in _browserBindingProperties which would provide a reasonable substitute for the accessed property without instantiating.  We have covered a good many of cases but whether we have them all remains to be seen.  We rely primarily on users reporting on the message displayed in the console when a tab has been instantiated outside of explicit user action (tab selection at this point).

In the case of messageManager access, we could add a trap in _browserBindingProperties which would prevent instantiation and throw an assertion notifying the user that they are attempting to send messages to lazy tabs and they first need to be explicitly instantiated by calling _insertBrowser (which could be made public [1]).  That would at least give the developer an awareness that sending messages to lazy tabs will have consequences (and maybe an awareness of the existence of lazy tabs and what they are about).  You also mentioned queuing messages or something like that which may be another option though I don't know what all that would entail or how feasible it is in reality.

This all brings up a good point to consider.  Instantiation through access of _browserBindingProperties has primarily been a safety catch to keep things from outright breaking if a xul browser property is accessed on an unbound browser.  It was necessary as the lazy-browser API was being developed and slowly brought into life, but it seems that instantiation this way is not meant to exist forever.  It seems that instantiation this way should eventually be done away with, and _browserBindingProperties should only be used to either provide substitute properties where it makes sense (eg, currentURI, contentTitle, audioMuted, etc.) or to alert the user they have done something they shouldn't be doing.  This would shift unintended instantiation of lazy tabs from "We'd like to try not to do this" to "This is not allowed", since unintended instantiation can have negative effects on UX, in some cases very much so.

[1] browser.tabs.update() maybe ??

Comment 5

[u462496]

(In reply to Kevin Jones from comment #4)
> In the case of messageManager access, we could add a trap in
> _browserBindingProperties which would prevent instantiation and throw an
> assertion notifying the user that they are attempting to send messages to
> lazy tabs and they first need to be explicitly instantiated by calling
> _insertBrowser (which could be made public [1]).

This should be for very good reason, and now that I think about it, I don't know if we should provide addons a way to instantiate browsers.  Is there ever a reason an addon couldn't wait until the user explicitly selects a tab before using sendMessage?

Comment 6

[Tomislav Jovanovic :zombie]

Wait, does this just instantiate the browser, or actually wakes up (restores session?) the tab?  (First being a memory/performance issue, and second a semantic change).

If it's just the former, there is nothing that could handle the message in that tab anyway, and we should just drop it (or perhaps reject the sendMessage promise).

Comment 7

[u462496]

(In reply to Tomislav Jovanovic :zombie from comment #6)
> Wait, does this just instantiate the browser, or actually wakes up (restores
> session?) the tab?  (First being a memory/performance issue, and second a
> semantic change).
> 
> If it's just the former, there is nothing that could handle the message in
> that tab anyway, and we should just drop it (or perhaps reject the
> sendMessage promise).

The makes lazy tabs non-lazy, and yes, there will be a memory/performance issue in doing that.

Comment 8

[u462496]

(In reply to Tomislav Jovanovic :zombie from comment #6)
> If it's just the former, there is nothing that could handle the message in
> that tab anyway

This is the reason the xul:browser gets instantiated - because messageManager does not exist on the naked browser.  The default action when xul:browser properties are accessed before the browser is bound is to bind it so they will be there.  As I have stated, this is intended as a safety catch fallback while all the bugs are getting worked out, and hopefully someday will be eliminated as all the scenarios are handled explicitly (even if by rejection).

Comment 9

[Tomislav Jovanovic :zombie]

So, currently calling sendMessage() for a tab that is pending restore already throws because there can't be any recipients if the page is not loaded.

So we should just avoid accessing the message manager of the tab in that case, to avoid instantiating the tab's <browser>.  In other words, this wouldn't be an observable change in behavior, except for performance.

Comment 10

[Tomislav Jovanovic :zombie]

Attachment: Bug 1377734 - Avoid instantiating a lazy tab from sendMessage,

Review commit: https://reviewboard.mozilla.org/r/167332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167332/

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8896058 [details]
Bug 1377734 - Avoid instantiating a lazy tab from sendMessage,

https://reviewboard.mozilla.org/r/167332/#review172534

::: toolkit/components/extensions/ExtensionParent.jsm:283
(Diff revision 1)
>     * @returns {object|null} The message manager matching the recipient if found.
>     */
>    getMessageManagerForRecipient(recipient) {
>      let {tabId} = recipient;
>      // tabs.sendMessage / tabs.connect
>      if (tabId) {

See bug 1388066, tabId can be zero on android.  If this is used on android please fix the use here as well, or drop a note on the other bug.

Comment 12

[Tomislav Jovanovic :zombie]

Comment on attachment 8896058 [details]
Bug 1377734 - Avoid instantiating a lazy tab from sendMessage,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167332/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f71999ab349
Avoid instantiating a lazy tab from sendMessage, r=mixedpuppy

Comment 14

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

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=122605089&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/b740506dadea2d566211acc7136d354effa2b5c1

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi :zombie,
I've been able to reproduce test failure locally on the android emulator and to collect a more useful stack trace for the issue from "adb logcat":

E/GeckoConsole( 7431): [JavaScript Error: "TypeError: tab.getAttribute is not a function
E/GeckoConsole( 7431): getMessageManagerForRecipient@resource://gre/modules/ExtensionParent.jsm:278:19
E/GeckoConsole( 7431): receiveMessage@resource://gre/modules/ExtensionParent.jsm:208:22
E/GeckoConsole( 7431): _handleMessage/</<@resource://gre/modules/MessageChannel.jsm:632:19
E/GeckoConsole( 7431): _handleMessage/<@resource://gre/modules/MessageChannel.jsm:631:9
E/GeckoConsole( 7431): _handleMessage@resource://gre/modules/MessageChannel.jsm:629:7
E/GeckoConsole( 7431): receiveMessage@resource://gre/modules/MessageChannel.jsm:163:5
E/GeckoConsole( 7431): " {file: "resource://gre/modules/MessageChannel.jsm" line: 634}]
E/GeckoConsole( 7431): _handleMessage/</<@resource://gre/modules/MessageChannel.jsm:634:11

The reason is definitely due to the different kind of return value of the `tabTracker.getTab` function (http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/mobile/android/components/extensions/ext-utils.js#270), which on Desktop returns a XUL <tab> element, while on Android returns an object with this prototype: http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/mobile/android/chrome/content/browser.js#3504

Once we decide which is the strategy that we prefer to solve this issue (e.g. make a different check on android or adapt the two kind of native tab objects to be able to use the same check for both the scenarios) we should also add a new android test similar to the new "browser/components/extensions/test/browser/browser_ext_tabs_lazy.js" for the android tabs (which have the concept of pending tab, implemented in the Tab prototype linked above from the Firefox for Android sources).

Comment 16

[Tomislav Jovanovic :zombie]

Comment on attachment 8896058 [details]
Bug 1377734 - Avoid instantiating a lazy tab from sendMessage,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167332/diff/2-3/

Comment 17

[Tomislav Jovanovic :zombie]

Thanks Luca 

> while on Android returns an object with this prototype:
Yeah, I totally spaced out that [1] says `browser` and not `tab.getAttribute()`.


> we should also add a new android test similar to the new
> "browser/components/extensions/test/browser/browser_ext_tabs_lazy.js" for
> the android tabs (which have the concept of pending tab, implemented in the
> Tab prototype linked above from the Firefox for Android sources).

So, a "lazy" tab is not the same as a tab "pending" session restore.  See bug 906076 and bug 1364847 for details, but in short, every "lazy" tab is also a "pending" tab (that is, page is not yet session restored), but a "lazy" tab doesn't even have an actual <browser>, and has a stub proxy object instead.  Accessing certain properties of that stub browser (like the message manager) instantiates the full <browser>, and that's what this bug is preventing.

Android only has "pending" tabs, but not the "lazy" kind yet (bug 1343090).  I'm still using the "pending" check because that makes sense semantically -- and it will probably work automagically when android gets lazy tabs, but the same test wouldn't work until then.

Comment 18

[Tomislav Jovanovic :zombie]

(In reply to Tomislav Jovanovic :zombie from comment #17)
> Yeah, I totally spaced out that [1] says `browser` and not `tab.getAttribute()`.

1) http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3655

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f7b063d6ff1
Avoid instantiating a lazy tab from sendMessage, r=mixedpuppy

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9f7b063d6ff1