Closed Bug 1029889 Opened 6 years ago Closed 3 years ago

about:home should use the search engine [chooser/picker/switcher] from the about:newtab page

Categories

(Firefox :: General, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED INVALID

People

(Reporter: clarkbw, Assigned: alexbardas)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

bug 962490 added a search field to the new tab page and during that process also created a search engine chooser for the search input described in attachment 8401003 [details] (at the bottom)

To maintain a consistent interface in terms of logos and interaction I'm suggesting we use the same chooser from the about:newtab page in the about:home page.  We've already done a large amount of work getting the new logo/image sizes cleared from partners required for the chooser.  Moving these images over to the about:home shouldn't incur any additional work other than making some partners aware of the change to the search input.

Timeline for this change besides the engineering work should give additional time for bizdev to inform our partners before this work gets uplifted.
Flags: firefox-backlog+
Summary: about:home should use the search engine chooser from the about:newtab page → about:home should use the search engine [chooser/picker/switcher] from the about:newtab page
Depends on: 962490
Points: --- → 3
OS: Mac OS X → All
Hardware: x86 → All
Duplicate of this bug: 1065324
Or better, about:home == about:newtab, one page for both situations.
Flags: qe-verify?
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Iteration: --- → 36.1
I've discussed with Matt about this a bit because it may not be as straightforward as it seemed to be.

* about:newtab is privileged and it's basically a xul document 
* about:home it's unprivileged and it's xhtml 

I didn't find any good way to reuse the xul panel from about:newtab to about:home. I'll add a styled select box instead of that panel in the first patch.

Possible downsides: 
* the onshow animation
* the panel top arrow

Bug 1051837 would be fixed by implementing that dropdown panel as a styled html select element.
FWIW, there's bug 776477 tracking work to convert about:newtab to an unprivileged HTML page.
Not sure what the timeline there is and how (if at all) that affects your work though.
Issue remains that there are two start pages, one is used for new tabs and one for new windows?
I think for ease and simplicity (both in code maintenance and user configuration) having one start page should be better.
Depends on: 873923
It is not finished yet, I still have to fix the tests, avoid the security error from about:home when displaying the logo and to properly show the panel. Being a very big patch I can use some feedback in the meantime.
Attachment #8509152 - Flags: feedback?(adw)
Updated the patch with the latest improvements + fixed tests
Attachment #8509152 - Attachment is obsolete: true
Attachment #8509152 - Flags: feedback?(adw)
Attachment #8509993 - Flags: feedback?(adw)
Hi Bobby. I'm running into a problem when trying to pass an Array Buffer from chrome process to an unprivileged content process (in about:home). The object is cloned here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js?rev=b81fb178319b#359.

"data" is an object and contains multiple properties, some of them are Array Buffers. 

What happens next in the flow is that a new blob is created from that array buffer, and then a urlobject (similar to here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js?rev=f7e5136269fa#205).

Unfortunately, the url cannot be loaded in the unprivileged process ("Security Error: Content at about:home may not load data from blob:null/5b281477-b78a-7043-b1f3-1cc190e298d9.")

What I did more was to individually clone the array buffer (considering that cloneInto is not recursive) and pass it as an individual argument. When I do this, I get "XrayWrapper denied access to property logo2xBuffer (reason: value not Xrayable). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information." . I've tried also using waiveXray but with no luck.

Do you have any idea I need to do in order to use that array buffer in the unprivileged context?
Flags: needinfo?(bobbyholley)
(In reply to Alex Bardas :alexbardas from comment #8)
> Hi Bobby. I'm running into a problem when trying to pass an Array Buffer
> from chrome process to an unprivileged content process (in about:home).

Is there actually a separate process involved here, or by "process" do you mean "compartment"?

> What happens next in the flow is that a new blob is created from that array
> buffer, and then a urlobject (similar to here:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/
> search.js?rev=f7e5136269fa#205).

Are you doing the URL.createObjectURL call in content or in chrome? You need to do it in content.

> Unfortunately, the url cannot be loaded in the unprivileged process
> ("Security Error: Content at about:home may not load data from
> blob:null/5b281477-b78a-7043-b1f3-1cc190e298d9.")

This suggests that you're calling it in chrome.

> What I did more was to individually clone the array buffer (considering that
> cloneInto is not recursive)

It is recursive. There should be no need for this.

> and pass it as an individual argument. When I do
> this, I get "XrayWrapper denied access to property logo2xBuffer (reason:
> value not Xrayable). See
> https://developer.mozilla.org/en-US/docs/Xray_vision for more information."
> .

What does the code that triggers this look like, exactly?

> I've tried also using waiveXray but with no luck.

Please don't do that unless you're sure you know what you're doing - it's a very good way to introduce security bugs. ;-)

> Do you have any idea I need to do in order to use that array buffer in the
> unprivileged context?

In general, you should just need to pass it in a structured clone, and then have content invoke URL.createObjectURL.
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #9)
> Is there actually a separate process involved here, or by "process" do you
> mean "compartment"?

Either way -- running the page in an e10s window or not -- we see the behavior Alex described.

To reiterate what he was saying, we have some ArrayBuffers that are created in chrome, in a JSM.  We assign them to some properties on a JS object and pass the object in a message to content.  A content script receives the message, does Cu.cloneInto as described below, and fires an event at the unprivileged page with the resulting object.  Then the page does URL.createObjectURL(new Blob([eventData.arrayBuf])) and sets background-image on a div with the resulting URI.

This is what the content script does after receiving the message from chrome:

  // `msg` is the message object received from chrome, `msg.data` is an object
  // with some properties that are ArrayBuffers
  let event = Cu.cloneInto({
    detail: {
      type: type,
      data: msg.data,
    },
  }, content);
  content.dispatchEvent(new content.CustomEvent("ContentSearchService", event));

With that, this error appears in the browser console when the page sets the background-image:

  Security Error: Content at about:home may not load data from blob:null/56e1f0d1-6a21-3a45-b6d7-b504ecb21aad.

Alex then tried doing an additional cloneInto on the array buffer properties, resulting in the "XrayWrapper denied access" error he mentioned, and then he tried waiveXray, but what I outlined above is how it normally works.

So, we are calling URL.createObjectURL from (unprivileged) content, and we are passing our ArrayBuffers in a structured clone.

Hope that makes things clearer.
Flags: needinfo?(bobbyholley)
(In reply to Drew Willcoxon :adw from comment #10)
>   let event = Cu.cloneInto({
>     detail: {
>       type: type,
>       data: msg.data,
>     },
>   }, content);
>   content.dispatchEvent(new content.CustomEvent("ContentSearchService",
> event));

This looks right.

> With that, this error appears in the browser console when the page sets the
> background-image:
> 
>   Security Error: Content at about:home may not load data from
> blob:null/56e1f0d1-6a21-3a45-b6d7-b504ecb21aad.

Hm, that error looks to me like the load is being denied for a policy reason, and has nothing to do with the contents of the underlying Blob. What happens if you URL.createObjectURL() from some image that came from the page, and has nothing to do with the structured clone?

If it works there (but not with the clone), please make a reduced testcase and I'll try to find time to take a look at it locally.
Flags: needinfo?(bobbyholley)
Thanks, Bobby.  It seems to be related to about: pages.  This patch (against fx-team/m-c) creates two new pages, about:arraybuffer-priv and about:arraybuffer-unpriv.  Both load the same arrayBuffer.xhtml, which converts a base-64-encoded Google logo to an array buffer, gets a new blob URL from the buffer, and then sets the page's background-image.  unpriv has URI_SAFE_FOR_UNTRUSTED_CONTENT set, priv doesn't.

priv displays the image as expected.  unpriv doesn't and reports the aforementioned "Security Error".  If you view arrayBuffer.xhtml over http or file://, the image is also displayed.

If it matters, on priv, unpriv, and via file://, the blob URL looks like blob:null/{GUID}.  Via http, it looks like blob:http://localhost:8000/{GUID}.

Do you know what might be going on?
Flags: needinfo?(bobbyholley)
Is the problem that the page is creating a blob it can't access -- the page is unprivileged, but the blob URL indicates the blob has the system principal?  That would kind of make sense in Alex's actual use case since the backing array buffer is created in chrome (even though the blob is created in content), but in my test case, the page is creating everything itself, so why would its buffer or blob have the system principal?
Depends on: 1088617
Thanks for the testcase Drew. This is a real bug in CAPS (see bug 1088617).

(In reply to Drew Willcoxon :adw from comment #13)
That would kind of make sense in Alex's actual use case since
> the backing array buffer is created in chrome (even though the blob is
> created in content)

The clone works by serializing + deserializing, at least for bonafide EcmaScript objects like ArrayBuffer. Once the clone happens, the ArrayBuffer is 100% content, and the fact that it was created by copying chrome data should be entirely unobservable.
Flags: needinfo?(bobbyholley)
Comment on attachment 8509993 [details] [diff] [review]
WIP 2 Reuse the search engine chooser + search form from newtab in abouthome + fixed existing tests

Review of attachment 8509993 [details] [diff] [review]:
-----------------------------------------------------------------

Here's my first round of comments.  I've only looked at the client searchFormUI.js so far.  I'll look at the ContentSearch.jsm changes next and then everything else after that.

Like we talked about in person, this bug is about adding the engine picker to about:home, but this patch goes a little beyond that and also handles searching (gSearch.search).  Eventually I'd like for about:home to use all of newtab's search code (well, I'm biased), but for this bug, let's not do more than is necessary, so let's factor out only the search picker part.  That reduces the scope of what we need to change here, but it would also let us use the picker in isolation later on if we want.

::: browser/base/content/searchFormUI.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Let's do this to preserve blame where possible:

hg cp browser/base/content/newtab/search.js browser/base/content/searchEnginePicker.js

(Also rename searchFormUI.js searchEnginePicker.js.)

@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +let gSearch = {

Let's make this a constructor rather than a singleton object, like how SearchSuggestionUIController is a constructor.  I'd suggest calling it SearchEnginePicker, but I'm open to alternatives.

Have it take a div element as a param that becomes this._nodes.logo (except you would call it this._node or something).

@@ +8,5 @@
> +
> +  currentEngineName: null,
> +
> +  // newtab / abouthome entrypoint
> +  entryPoint: "",

With my suggestion above about scoping this only to the picker, we can get rid of this.

@@ +22,5 @@
> +    this._send("GetState");
> +  },
> +
> +  showPanel: function () {
> +    this._nodes.logo.setAttribute("active", true);

Clicking the logo while the panel is already open should close the panel.  So this method should be renamed togglePanel, and it should not send the message if the logo is already active.  (It would close the panel once it receives RemoveLogoActiveState.)

Let's also rename "active" to "panelOpen" or something like that.  "Active" is pretty ambiguous.

@@ +24,5 @@
> +
> +  showPanel: function () {
> +    this._nodes.logo.setAttribute("active", true);
> +    let box = this._nodes.logo.getBoundingClientRect();
> +    this._send("OpenSearchPanel", {

Call this message OpenPickerPanel.  "Search" is redundant because all of the messages handled by the content search service are related to search.

@@ +31,5 @@
> +      y: box.bottom
> +    });
> +  },
> +
> +  search: function (event) {

Can remove this too.

@@ +78,5 @@
> +    }
> +  },
> +
> +  onState: function (data) {
> +    this._newEngines = data.engines;

_newEngines was only needed to build the panel, but now that content isn't doing that, it's not necessary anymore.

@@ +96,5 @@
> +      this._setCurrentEngine(engineName);
> +    }
> +  },
> +
> +  onFocusInput: function () {

Can remove this too, but we'll need some way for the page to focus its input after the user picks an engine from that page's picker.  I think I have an idea about that, and I'll mention it later.

@@ +100,5 @@
> +  onFocusInput: function () {
> +    this._nodes.text.focus();
> +  },
> +
> +  onRemoveLogoActiveState: function () {

Let's call this message PickerPanelClosed.

@@ +107,5 @@
> +
> +  _nodeIDSuffixes: [
> +    "form",
> +    "logo",
> +    "text",

And then we only need to keep track of `logo`.

@@ +114,5 @@
> +  _nodes: {},
> +
> +  _initWhenInitalStateReceived: function () {
> +    this._nodes.form.addEventListener("submit", e => this.search(e));
> +    this._nodes.logo.addEventListener("click", e => this.showPanel());

I think we can simplify this a lot.  We can add this click listener when the picker object is created (i.e., in the constructor I proposed) instead of waiting for the initial state event.  The reason that gSearch currently waits is that without state, gSearch doesn't know what the engines are, so it can't build the panel, so it doesn't allow the user to click to open the panel.  But now that chrome is building the panel, it doesn't make sense for content to wait until it receives the message.  (But if we do what I suggest, then we need to make sure that chrome always opens the panel in response to the open-panel message, even if it's not ready to do so when it receives the message.  Otherwise we can end up in a state where the page thinks the panel is open but it's not.)

So that means we can remove _initWhenInitalStateReceived and _initialStateReceived.

@@ +170,5 @@
> +    }
> +    this._suggestionController.engineName = engine.name;
> +  },
> +
> +  _setSuggestionControllerTimeout: function (timeout) {

We can remove this too.
I finally took a good look at FormValidationHandler.jsm and bug 691601 after talking about it with you, Alex.  I'm not sure what you or I meant exactly when we said we should "use" that JSM, but we should probably make it generic so that it can open any given panel over content, and then use that here as much as possible.  i.e., s/FormValidation/ContentPanel/g, and then remove the only part that seems to be specific to the form validation popup, where _showPopup sets some textContent to a string in the message from content.  We shouldn't ever want to show more than one panel at a time over content, so using a single "anchor" for all such panels (tabbrowser.formValidationAnchor, which should be renamed) ought to be fine.

I still plan to comment on the rest of your patch next.
Hi Alex, can you mark this bug for QE verification.
Flags: needinfo?(abardas)
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(abardas)
(In reply to Drew Willcoxon :adw from comment #16)
> I finally took a good look at FormValidationHandler.jsm and bug 691601 after
> talking about it with you, Alex.  I'm not sure what you or I meant exactly
> when we said we should "use" that JSM, but we should probably make it
> generic so that it can open any given panel over content, and then use that
> here as much as possible.  i.e., s/FormValidation/ContentPanel/g, and then
> remove the only part that seems to be specific to the form validation popup,
> where _showPopup sets some textContent to a string in the message from
> content.  We shouldn't ever want to show more than one panel at a time over
> content, so using a single "anchor" for all such panels
> (tabbrowser.formValidationAnchor, which should be renamed) ought to be fine.
> 
> I still plan to comment on the rest of your patch next.

Should I file a new bug for making a generic ContentPanel module (that would also block this bug)? Doing that in a new bug seems like a better idea for me.
Flags: needinfo?(adw)
Yes, I think so.  I think content ought to be able to send it messages to open panels, but I think chrome should also be able to call into it on behalf of content.  I'm typing up a big comment that elaborates on that.
Flags: needinfo?(adw)
Comment on attachment 8509993 [details] [diff] [review]
WIP 2 Reuse the search engine chooser + search form from newtab in abouthome + fixed existing tests

Review of attachment 8509993 [details] [diff] [review]:
-----------------------------------------------------------------

These comments are about the ContentSearch.jsm changes.  I'm proposing some pretty big changes, and I've probably missed some details, but this is how I think it should work.

We should take this opportunity to XBL'ify the search panel.  We should add it to http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/search.xml, since the ID of the <bindings> there is "SearchBindings", which fits.  All the code from your ContentSearch.jsm changes that sets up the panel should be in the XBL.  There's no need to use ContentSearch._currentStateObj or array buffers when building the panel.  Just use Services.search directly.  The panel needs to keep itself updated when engines change; it should add an nsIObserver, similar to how ContentSearch adds itself as an observer (_onObserve), and then rebuild itself after a change the next time it opens.  It should fire a custom "close" event when it's closed.  event.detail.selectedEngine should be the picked engine name; if there was no picked engine, then that property is undefined or null.

Fleshing out what I said in my previous comment, I think the message from content to open the panel should still go through ContentSearch, instead of FormValidationHandler directly, so that ContentSearch is able to observe the interaction and do things like focus the search input after an engine is picked.  Therefore ContentSearch should be able to call some method on FormValidationHandler that opens the panel for the appropriate xul:browser in msg.target.  So the flow is: the page sends an open-panel message to ContentSearch; ContentSearch gets the panel in browser.xul, which it looks up based on msg.target; ContentSearch adds a close listener on the panel; ContentSearch calls into FormValidationHandler to open the panel at the position specified in the open-panel message; ContentSearch's close listener removes itself, and if event.detail.selectedEngine is defined, it sets the current engine and sends a message to the page to focus its search input.

What do you think?
Attachment #8509993 - Flags: feedback?(adw)
Iteration: 36.1 → 36.2
See Also: → 1091117
Depends on: 1091930
Assignee: alex.bardas → nobody
Status: ASSIGNED → NEW
Iteration: 36.2 → ---
Assignee: nobody → alex.bardas
Status: NEW → ASSIGNED
This was implemented in a different way.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.