Last Comment Bug 771395 - show origin of page in window title when it is different from webapp's origin
: show origin of page in window title when it is different from webapp's origin
Status: VERIFIED FIXED
[blocking-webrtdesktop1+], [qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Webapp Runtime (show other bugs)
: 14 Branch
: All All
: P1 normal
: Firefox 16
Assigned To: Myk Melez [:myk] [@mykmelez]
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: 741955
  Show dependency treegraph
 
Reported: 2012-07-05 17:40 PDT by Myk Melez [:myk] [@mykmelez]
Modified: 2016-03-21 12:39 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1: show different origin in window title (5.17 KB, patch)
2012-07-05 17:40 PDT, Myk Melez [:myk] [@mykmelez]
no flags Details | Diff | Review
bypass install-with-runtime in test mode (for real this time) (1.37 KB, patch)
2012-07-06 14:37 PDT, Drew Willcoxon :adw
no flags Details | Diff | Review
patch v2: show different origin in window title (9.26 KB, patch)
2012-07-06 18:09 PDT, Myk Melez [:myk] [@mykmelez]
no flags Details | Diff | Review
patch v3: address issues (8.08 KB, patch)
2012-07-09 17:49 PDT, Myk Melez [:myk] [@mykmelez]
felipc: review+
myk: checkin+
Details | Diff | Review

Description Myk Melez [:myk] [@mykmelez] 2012-07-05 17:40:49 PDT
Created attachment 639534 [details] [diff] [review]
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] 2012-07-05 17:56:16 PDT
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 2012-07-05 18:13:05 PDT
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 2012-07-06 14:08:21 PDT
(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 2012-07-06 14:24:51 PDT
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 2012-07-06 14:37:29 PDT
Created attachment 639802 [details] [diff] [review]
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] 2012-07-06 18:09:33 PDT
Created attachment 639875 [details] [diff] [review]
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] 2012-07-06 18:19:11 PDT
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: gavin@gavinsharp.com] 2012-07-07 08:33:33 PDT
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] 2012-07-09 17:49:47 PDT
Created attachment 640461 [details] [diff] [review]
patch v3: address issues

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] 2012-07-10 15:09:01 PDT
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] 2012-07-10 20:31:43 PDT
https://hg.mozilla.org/mozilla-central/rev/5bb5b5dd4960
Comment 12 Jason Smith [:jsmith] 2012-07-12 15:10:47 PDT
Verified on Windows 7, OS X 10.7, Ubuntu 12.

Note You need to log in before you can comment on or make changes to this bug.