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

VERIFIED FIXED in Firefox 16

Status

Firefox Graveyard
Webapp Runtime
P1
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: myk, Assigned: myk)

Tracking

14 Branch
Firefox 16

Details

(Whiteboard: [blocking-webrtdesktop1+], [qa!])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #639534 - Flags: feedback?(adw)
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
blocking-kilimanjaro: --- → ?
tracking-firefox16: --- → ?

Comment 2

5 years ago
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.

Updated

5 years ago
QA Contact: jsmith

Comment 3

5 years ago
(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

5 years ago
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

5 years ago
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?

Updated

5 years ago
Priority: -- → P1
Whiteboard: [blocking-webrtdesktop1+]
(Assignee)

Comment 6

5 years ago
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!
Attachment #639534 - Attachment is obsolete: true
Attachment #639802 - Attachment is obsolete: true
Attachment #639534 - Flags: feedback?(adw)
Attachment #639875 - Flags: review?(felipc)
Attachment #639875 - Flags: review?(ctalbert)
(Assignee)

Comment 7

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

https://tbpl.mozilla.org/?tree=Try&rev=9ee5e0c0b661
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.
(Assignee)

Comment 9

5 years ago
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.
Attachment #639875 - Attachment is obsolete: true
Attachment #639875 - Flags: review?(felipc)
Attachment #639875 - Flags: review?(ctalbert)
Attachment #640461 - Flags: review?(felipc)
Attachment #640461 - Flags: review?(felipc) → review+
(Assignee)

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb5b5dd4960
Attachment #640461 - Flags: checkin+

Updated

5 years ago
Whiteboard: [blocking-webrtdesktop1+] → [blocking-webrtdesktop1+], [qa+]
https://hg.mozilla.org/mozilla-central/rev/5bb5b5dd4960
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Verified on Windows 7, OS X 10.7, Ubuntu 12.
Whiteboard: [blocking-webrtdesktop1+], [qa+] → [blocking-webrtdesktop1+], [qa!]

Updated

5 years ago
Status: RESOLVED → VERIFIED
tracking-firefox16: ? → ---
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.