771395 - show origin of page in window title when it is different from webapp's origin

Status: VERIFIED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: show different origin in window title

When the origin of a page loaded into a webapp window is different from the origin of the webapp, the runtime should show the origin of the page in chrome, according to the runtime security review, as tracked by bug 741955.

Here's a patch that does that, and it includes a webapprt chrome mochitest.  But the mochitest prompts me to install the test app instead of installing it without prompting.  Not sure what's going on there, so requesting feedback from adw on that issue before I ask someone for review of the patch itself.

Comment 1

[Jason Smith [:jsmith]]

firefox16 tracking and k9o nomination - security wants this fixed for the 1st release of the web runtime, and the associated sec review action item Curtis clarified to be needed

Comment 2

[Drew Willcoxon :adw]

Are you using the final version of the test framework patch, the one that landed?  It sounds like you're hitting the install-within-the-runtime path, which I only disabled in the final version of the test framework patch.

I applied your patch to my week-old mozilla-inbound tree and your test passed.  Then I tried again with a freshly pulled tree, but the patch didn't apply cleanly.

Comment 3

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #2)
> Then I tried again with a freshly pulled tree, but the patch didn't apply cleanly.

Sorry, this was my fault.  However, even after fixing that your test still passes for me (on mozilla-inbound).

Comment 4

[Drew Willcoxon :adw]

Oh, when you run two tests, the installation dialog pops up for the second.  I was running your test by itself.  Let me see what's going on.

Comment 5

[Drew Willcoxon :adw]

Attachment: bypass install-with-runtime in test mode (for real this time)

I made a dumb mistake (shocking).  Would you like to include this in your patch here, or should I attach this to a separate bug?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: show different origin in window title

Ok, here's a patch that's ready for review.  In addition to fixing this bug, it refactors references to the <browser> element via a new gBrowser global and makes MochiTest robust against the presence of a gBrowser global that isn't a <tabbrowser>.

Requesting review from ctalbert on the MochiTest changes (to testing/mochitest/browser-test.js).

Requesting review from felipe on the rest of the patch.

A few issues:

1. As I understand it, nsIWebProgressListeners added with nsIWebProgress.NOTIFY_LOCATION shouldn't be notified of other kinds of progress, but both listeners in this patch seem to be getting notified about all progress, which is why I define stub methods for all notifications.

2. It isn't clear that I need to remove the progress listeners, but Firefox's browser.js does it, so I do it in webapp.js; and I just added a MochiTest in another change that needed an observer removed <https://hg.mozilla.org/mozilla-central/rev/654181827f06>, even though in theory it shouldn't have, so I do it in this MochiTest, too.


(In reply to Drew Willcoxon :adw from comment #5)
> I made a dumb mistake (shocking).  Would you like to include this in your
> patch here, or should I attach this to a separate bug?

No worries, Drew, I've included it in this patch!

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Pushed to tryserver with mochitests enabled to make sure my changes don't break them:

https://tbpl.mozilla.org/?tree=Try&rev=9ee5e0c0b661

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 639875 [details] [diff] [review]
patch v2: show different origin in window title

I don't think it's a good idea to re-use the name "gBrowser" to refer to a <browser> rather than a <tabbrowser> - it will lead to a lot of confusion. Can you use another name?

(In reply to Myk Melez [:myk] [@mykmelez] from comment #6)
> 1. As I understand it, nsIWebProgressListeners added with
> nsIWebProgress.NOTIFY_LOCATION shouldn't be notified of other kinds of
> progress, but both listeners in this patch seem to be getting notified about
> all progress, which is why I define stub methods for all notifications.

browser.xml's version of addProgressListener just doesn't pass on the notify flags - that seems like something we could fix relatively easily, but as an alternative you could bypass it and just call browser.webProgress.addProgressListener directly.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: address issues

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> I don't think it's a good idea to re-use the name "gBrowser" to refer to a
> <browser> rather than a <tabbrowser> - it will lead to a lot of confusion.
> Can you use another name?

I think it's a worse idea to name webapprt/ symbols suboptimally to avoid potential confusion with similarly-named symbols in browser/, and we should generally refrain from doing that, as engineers pay the cost of suboptimal names every time they interact with the code, while only some of them some of the time will pay the cost of a potentially confusing name.

But I can use another name, and it does simplify review of the patch, since it means Clint doesn't need to review the changes to MochiTest.  In this update, I call it gAppBrowser.


> (In reply to Myk Melez [:myk] [@mykmelez] from comment #6)
> > 1. As I understand it, nsIWebProgressListeners added with
> > nsIWebProgress.NOTIFY_LOCATION shouldn't be notified of other kinds of
> > progress, but both listeners in this patch seem to be getting notified about
> > all progress, which is why I define stub methods for all notifications.
> 
> browser.xml's version of addProgressListener just doesn't pass on the notify
> flags - that seems like something we could fix relatively easily, but as an
> alternative you could bypass it and just call
> browser.webProgress.addProgressListener directly.

Nice catch, and good idea to fix the <browser> widget's addProgressListener method!  But I'll do that separately and have filed bug 772299 on it.  In the meantime, I've implemented the workaround you recommend.


In this patch, I also fixed an issue with webapp.js's registration of the progress listener.  Due to the different codepath for chrome webapprt tests, it was trying to register the listener multiple times, once per test app install.

I fixed the problem by explicitly registering the listener once, when the first app is installed (it can't happen before then because the progress listener needs access to the app manifest to determine whether the new location has the same as or a different origin than the app).

Note that the progress listener will still get called the second and subsequent times the harness loads a test file in addition to each time a test file installs a test app.  That doesn't currently cause problems, though; and fixing it is involved, because the harness loads test files and installs test apps in the same window.

One solution to the problem would be for the window to listen for test file completion and unregister the progress listener (along with undoing any other necessary app initialization), so initialization/uninitialization happens each time a test file is loaded/completes.

Another would be for CommandLineHandler.js to open a special window that loads test files and then opens separate app windows for each app a test file installs, closing the windows once the test file completes.  We might want to do something like this, but it's orthogonal to this work, so I'll file a separate bug on it.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 640461 [details] [diff] [review]
patch v3: address issues

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb5b5dd4960

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bb5b5dd4960

Comment 12

[Jason Smith [:jsmith]]

Verified on Windows 7, OS X 10.7, Ubuntu 12.