Re-enable social API tests in e10s mode

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 48
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox47 affected, firefox48 fixed)

Details

(Whiteboard: [e10s])

Attachments

(1 attachment, 1 obsolete attachment)

'document-element-inserted' is not fired for chat windows, but in content scripts, so currently the navigator.mozSocial object is not available in chat windows.

Therefore 'browser_social_chatwindow.js', 'browser_social_chatwindow_resize.js' and 'browser_social_chatwindowfocus.js' have been disabled
Can you provide an STR or some other details of what you saw or how you validated this?  I do see 'document-element-inserted' fired for chat windows in both e10s and non-e10s.  I do see other errors in the chat tests, but they are unrelated to mozSocial.  Hello chat windows do not get mozSocial because it doesn't use socialapi.
Flags: needinfo?(mdeboer)
I'm just going to make this work.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Iteration: --- → 47.3 - Mar 7
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Created attachment 8725266 [details] [diff] [review]
Patch v1: re-enable tests
Attachment #8725266 - Flags: review?(mixedpuppy)
Summary: Re-attach navigator.mozSocial → Re-enable social API tests in e10s mode
Comment on attachment 8725266 [details] [diff] [review]
Patch v1: re-enable tests

- Changing the function signature in a non-compatible way to support test-only use.  

- this is a web accessible api and I'm not certain we want to provide this way to have chrome steal focus.

If this is to make tests pass and we must do something like this, I'd rather see a wrapper in the tests and leave the api as-is.
Attachment #8725266 - Flags: review?(mixedpuppy) → review-

Updated

3 years ago
Blocks: 984139
tracking-e10s: --- → +
(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> Comment on attachment 8725266 [details] [diff] [review]
> Patch v1: re-enable tests
> 
> - Changing the function signature in a non-compatible way to support
> test-only use.  

I didn't change it in a non-compatible way; regular two-argument invocation is still supported explicitly.

> - this is a web accessible api and I'm not certain we want to provide this
> way to have chrome steal focus.

The problem is that we're returning `chatWin` to content - when the content is running in a separate process (e10s), the `chatWin` is a CPOW and virtually useless. In fact, I think we should think about dropping the feature of returning chrome object back to content, not in the least because it's a security risk. From `chatWin`, scripts have access to any part of the browser chrome and can do whatever they like to it.

> If this is to make tests pass and we must do something like this, I'd rather
> see a wrapper in the tests and leave the api as-is.

I guess I'm contesting whether we should keep the API in its current state. I moved the .focus() call to chrome, because it threw an error when called in the callback in content.

Since we should consider the barrier between chrome and content as absolute and the mozSocial API object is not easily stubbed from chrome, I'd like to ask you what kind of wrapper you'd think of?
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> Can you provide an STR or some other details of what you saw or how you
> validated this?  I do see 'document-element-inserted' fired for chat windows
> in both e10s and non-e10s.

I also don't remember/ know where that came from. In my patch I could simple re-enable two tests without any changes. They just worked! It must've been an artifact of the fx-team checkout I was running at the time...
(In reply to Mike de Boer [:mikedeboer] from comment #6)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> > Comment on attachment 8725266 [details] [diff] [review]
> > Patch v1: re-enable tests
> > 
> > - Changing the function signature in a non-compatible way to support
> > test-only use.  
> 
> I didn't change it in a non-compatible way; regular two-argument invocation
> is still supported explicitly.

Sorry, I noticed the signature change and the addition of focus in the api, glossed over that.

> > - this is a web accessible api and I'm not certain we want to provide this
> > way to have chrome steal focus.
> 
> The problem is that we're returning `chatWin` to content - when the content
> is running in a separate process (e10s), the `chatWin` is a CPOW and
> virtually useless. In fact, I think we should think about dropping the
> feature of returning chrome object back to content, not in the least because
> it's a security risk. From `chatWin`, scripts have access to any part of the
> browser chrome and can do whatever they like to it.

You made that change in bug 1154277, it shouldn't have been done.  The intent of that api is to work similar to window.open, returning a handle to the opened window.  Looks like that may have been uplifted through to beta as well.

> > If this is to make tests pass and we must do something like this, I'd rather
> > see a wrapper in the tests and leave the api as-is.
> 
> I guess I'm contesting whether we should keep the API in its current state.
> I moved the .focus() call to chrome, because it threw an error when called
> in the callback in content.

I'm guessing because the content window was no longer returned per above.

So my question is, is the focus failure you're trying to fix here due to the bug introduced in 1154277 or due to a real e10s limitation?
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> I'm guessing because the content window was no longer returned per above.
> 
> So my question is, is the focus failure you're trying to fix here due to the
> bug introduced in 1154277 or due to a real e10s limitation?

It doesn't really matter how the object is passed back down to content, but that it's passed back at all.
Returning chatWin to the caller, which lives in the content process, it has to be wrapped as a CPOW, because it's a chrome element that lives in the main process. That's why `chatWin.focus()` would throw an error, regardless if it's returned from the function invocation or as an argument to the callback.

So I'd say this is a real e10s limitation.
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> > I'm guessing because the content window was no longer returned per above.
> > 
> > So my question is, is the focus failure you're trying to fix here due to the
> > bug introduced in 1154277 or due to a real e10s limitation?
> 
> It doesn't really matter how the object is passed back down to content, but
> that it's passed back at all.
> Returning chatWin to the caller, which lives in the content process, it has
> to be wrapped as a CPOW, because it's a chrome element that lives in the
> main process. That's why `chatWin.focus()` would throw an error, regardless
> if it's returned from the function invocation or as an argument to the
> callback.
> 
> So I'd say this is a real e10s limitation.

chatWin was not passed back before the change in bug 1154277, chatWin.contentWindow was.  contentWindow.focus() shouldn't have thrown an error but I could be wrong about that.  I agree that chatWin should not be passed back to content.  I'm only opposed to changing the api for the wrong reason (ie. covering up the change in 1154277) which in this case I feel is happening.  I'm not clear why the change in 1154277 happened, but IMO that needs to be fixed first.
(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> chatWin was not passed back before the change in bug 1154277,
> chatWin.contentWindow was.  contentWindow.focus() shouldn't have thrown an
> error but I could be wrong about that.  I agree that chatWin should not be
> passed back to content.  I'm only opposed to changing the api for the wrong
> reason (ie. covering up the change in 1154277) which in this case I feel is
> happening.  I'm not clear why the change in 1154277 happened, but IMO that
> needs to be fixed first.

Because `.contentWindow` and `.contentDocument` are CPOWs as well, when the chat document is running in the content process - like Hello will.
I understand your reasoning, I think: So contentWindow is the window object in the content process, which we want to pass back to another document that lives in the same process, which should just work, right? Well, the point here is that

a) We're doing: content asks "Can you open chatWin, plz", chrome process goes "OHAI", opens a chatbox, waits for it to load and passes back whatever it can, but in this case it needs to go back to a content process (which poor script in chrome process is unaware of) and so content gets back a CPOW, does "NOMNOM", but it explodes.
b) The fact that both docShells (sidebar and chatwindow documents) live in the same content process and would share the same address space doesn't mean they can assume they're siblings. In fact, in the future we're intending to scale up our count of content processes, so that's where this model definitely comes to an end.

In other words, accessing `.contentWindow` or `.contentDocument` is evil in e10s lingo. Passing the objects around by reference as part of a web-facing API is, well, evil as well :o)
I still think that the issue if returning chatWin needs to be addressed.  It was uplifted, is in 45, and if it is a security issue as you mentioned earlier, needs to be fixed (though I'm uncertain is is really a security issue).  That is a separate issue from addressing any api change or e10s.

Then, lets evaluate how the api changes.  I think we can be certain that it no longer returns any object in the callback, though the callback may be useful to maintain.  

The reason we did not do focus in the api before was that a chat window could be opened up from a background worker.  We didn't want the focus to be stolen if the user was in the midst of typing in a form, that could unintentionally leak something private into the chat window.

I think the behavior that is desirable is:
- if a user event results in opening a chat window, it should be focused
- if a background api call opens a chat window, it should not be focused

In mozsocial.share, we use nsIDOMWindowUtils.isHandlingUserInput to determine if we're in a user event.  We can do that in openChatwindow as well:

openChatWindow: {
  enumerable: true,
  configurable: true,
  writable: true,
  value: function(toURL, callback) {
    let url = targetWindow.document.documentURIObject.resolve(toURL);
    let dwu = chromeWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                         .getInterface(Ci.nsIDOMWindowUtils);
    openChatWindow(targetWindow, provider, url, chatWin => {
      if (chatWin && dwu.isHandlingUserInput) {
        chatWin.focus();
      }
      if (callback)
        callback(!!chatWin);
    });
  }

The tests change to test for:

- simple API call does not focus chat window
- user event then opening does focus chat window
- callback only indicates whether chat window was opened

There shouldn't be any real blowback on the api change here, but documentation needs to note it.
Created attachment 8729005 [details] [diff] [review]
Patch v2: re-enable tests
Attachment #8725266 - Attachment is obsolete: true
Attachment #8729005 - Flags: review?(mixedpuppy)
Comment on attachment 8729005 [details] [diff] [review]
Patch v2: re-enable tests

cool.  I think I was moving towards some similar changes in the tests in bug 898706
Attachment #8729005 - Flags: review?(mixedpuppy) → review+
https://hg.mozilla.org/integration/fx-team/rev/2bef59a4476a99e0796bde9502c1bf0744f97182
Bug 1245798: re-enable browser_social_activation.js, browser_social_chatwindowfocus.js and browser_social_contextmenu.js. r=mixedpuppy

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2bef59a4476a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.