Closed Bug 462856 Opened 16 years ago Closed 15 years ago

offline app notification code doesn't handle subframes

Categories

(Firefox :: General, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: Gavin, Assigned: dcamp)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [has patch])

Attachments

(1 file, 3 obsolete files)

There are a few problems with the patch from bug 394392.

1) OfflineApps.allowSite() uses gBrowser.selectedBrowser.webNavigation.currentURI, which isn't necessarily the URI of the site that originally made the request.

2) _startFetching similarly uses content.document.documentElement which won't be the right element if frames are involved.

3) _startFetching creates a URI for content.location.href, which is unnecessary (could use document.documentURIObject/baseURIObject)

These should all be fixed by passing in the |browser| to allowSite/disallowSite through a closure in offlineAppRequested, and getting the relevant properties (currentURI, contentDocument) off of that. 

And one stylistic nit:
4) the gBrowser.currentURI is equivalent to gBrowser.selectedBrowser.webNavigation.currentURI, and browser.currentURI is equivalent to browser.webNavigation.currentURI.
Flags: blocking-firefox3.1?
Assignee: nobody → dcamp
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Attached patch fix (obsolete) — Splinter Review
The other problem is that we don't actually notice the manifest attribute in subframes.  This code uses a DOMContentLoaded handler to notice subframes loading.  Not sure if there's a better way to do that...
Attachment #350940 - Flags: review?(gavin.sharp)
Whiteboard: [has patch][needs review gavin]
Priority: -- → P2
Blocks: 462854
I don't really like the idea of adding a DOMContentLoaded handler to our frontend code to handle this. Scheduling updates based on the presence of a manifest attribute doesn't seem like something the front end should be responsible for, and prompting the user to allow a site to use offline storage should probably be triggered using an event fired at the relevant content window (along the lines of the DOMPopupBlocked event).

I'm not sure where the best place to move this code would be, but perhaps bzbarsky can advise. I'm not sure if it's reasonable to suggest that we should try to address these concerns for 1.9.1, because I'm not sure exactly how much work that would involve. dcamp, thoughts?
Uh... so why does all this code live in browser.js and not in Gecko anyway?  I don't just mean the DOMContentLoaded handler, but _any_ of this code...
As in, isn't offline app caching a Gecko feature, not a Firefox one?
As for the main question, if someone can fill me in on the background of this code (mail, design docs, catch me on irc, whatever), that would be nice.  I can try to read through bug 394392, but if there's a summary somewhere...
(In reply to comment #2)
> I don't really like the idea of adding a DOMContentLoaded handler to our
> frontend code to handle this. Scheduling updates based on the presence of a
> manifest attribute doesn't seem like something the front end should be
> responsible for,

For the most part it isn't - once the permission is set up, scheduling cache updates happens in the backend code entirely.  The frontend code does schedule an initial cache update after the user grants permission (because we're done with the loading process that usually schedules the update at that point).

> and prompting the user to allow a site to use offline storage
> should probably be triggered using an event fired at the relevant content
> window (along the lines of the DOMPopupBlocked event).

That should be easy enough.  I'm attaching an interdiff that uses something like this (though likely in an inopportune place).

(In reply to comment #3)
> Uh... so why does all this code live in browser.js and not in Gecko anyway?  I
> don't just mean the DOMContentLoaded handler, but _any_ of this code...

The code in browser.js is mostly UI code - showing disk usage warnings and notifying the user that a page lists a manifest but doesn't have permission.  (as mentioned before, the exception is kicking off an initial cache attempt once the user has granted permission).
(In reply to comment #5)
> As for the main question, if someone can fill me in on the background of this
> code (mail, design docs, catch me on irc, whatever), that would be nice.  I can
> try to read through bug 394392, but if there's a summary somewhere...

So the basic problem to solve is deciding when showing the "This site (www.whatever.com) can store information offline, click here to give it permission" infobar.  In 3.0 (when we only cached toplevel documents), we just checked the toplevel document for a manifest and showed that bar as necessary.

But that's not really feasible when we have iframes in the picture.  The patch used DOMContentLoaded to check documents for a manifest attribute, gavin is suggesting adding some sort of similar event that is only fired for pages with a manifest.
Attached patch hacky fix (obsolete) — Splinter Review
This just fires a DOMApplicationManifest event right after DOMContentLoaded if the document has a manifest.
I'd rather not add more events that content can see.  If we only fire the event to chrome, fine, but firing random events we make up to content, and putting them in the DOM spec's event namespace at that, is just not cool.

What exactly is the issue with DOMContentLoaded?  Performance concerns?
Yes, mostly performance. I'd like to avoid the need to poll every single document loaded in front-end code when it wouldn't be much harder to let Gecko notify instead (a chrome-only event would be fine).

(My thinking may be a little tainted here from having spent a bit too much time lately worrying about Fennec perf, where even just dealing with xpconnect overhead to retrieve "doc.documentElement" twice is not insignificant.)
I'd be ok with firing a new event with a Moz prefix directly at the chrome event handler.  It'd be really nice to have a helper for doing that sort of thing, as far as that goes; we fire all sorts of stuff at the window that I think we should stop firing there.
Comment on attachment 361892 [details] [diff] [review]
hacky fix

If you do this, you at least should use capturing phase and call .stopPropagation() so that the event doesn't propagate to content.

The event name should start with Moz.
Using capturing is not good enough, because then content will see the event in an embedding that doesn't have this UI (e.g. any other Gecko-based browser).  If we go with a new event we really do need to fire it directly on the chrome event handler.
Firing on chrome event handler is possible too.
Something like this (excluding null checks):
nsCOMPtr<nsIDOMEvent> domEvent;
CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(domEvent))
domEvent->InitEvent("MozApplicationManifest", PR_TRUE, PR_TRUE);
nsCOMPtr<nsIPrivateDOMEvent> pievent = do_QueryInterface(domEvent);
pievent->SetTarget(this); // 'this' is nsIDocument*
pievent->SetTrusted(PR_TRUE);
GetWindow()->GetChromeEventHandler()->DispatchDOMEvent(nsnull, domEvent, nsnull, nsnull);

That way the target/originalTarget of the event is still document.

Feel free to add a helper method to nsContentUtils.
Reworked to send a MozApplicationCache to the chrome event handler, with an nsContentUtils helper.
Attachment #361892 - Attachment is obsolete: true
Attachment #362190 - Flags: review?(bzbarsky)
Attachment #362190 - Attachment is patch: true
Attachment #362190 - Attachment mime type: application/octet-stream → text/plain
I assume that's an interdiff against the first patch in this bug and that you just want me to review the content changes, not the browser change?

Was there a reason to not use DispatchDOMEvent() and instead to QI to nsIDOMEventTarget on the chrome event handler?
(In reply to comment #16)
> I assume that's an interdiff against the first patch in this bug and that you
> just want me to review the content changes, not the browser change?

Yeah (unless you feel like looking at browser changes too..)

> Was there a reason to not use DispatchDOMEvent() and instead to QI to
> nsIDOMEventTarget on the chrome event handler?

Well, DispatchDOMEvent() is marked @deprecated in the header (though the comment does kinda imply that it isn't deprecated for internal use).  I can change that up.
With DispatchDOMEvent() and the QI removed and the null-check fixed accordingly, looks good to me.  I'd rather not look at the browser parts.  ;)
Comment on attachment 362190 [details] [diff] [review]
fire MozApplicationManifest to the chrome evt handler

>@@ -5336,16 +5326,22 @@ var OfflineApps = {

>+  handleEvent: function(event) {
>+    if (event.type == "MozApplicationManifest") {
>+      OfflineApps.offlineAppRequested(event.originalTarget.defaultView);

this.offlineAppRequested...?
Comment on attachment 350940 [details] [diff] [review]
fix

>       var buttons = [{
>         label: bundle_browser.getString("offlineApps.allow"),
>         accessKey: bundle_browser.getString("offlineApps.allowAccessKey"),
>-        callback: function() { OfflineApps.allowSite(); }
>+        callback: function() {
>+            for (var i = 0; i < notification.documents.length; i++) {
>+              OfflineApps.allowSite(notification.documents[i]);
>+            }
>+          }
>       },{
>         label: bundle_browser.getString("offlineApps.never"),
>         accessKey: bundle_browser.getString("offlineApps.neverAccessKey"),
>-        callback: function() { OfflineApps.disallowSite(); }
>+        callback: function() {
>+            for (var i = 0; i < notification.documents.length; i++) {
>+              OfflineApps.disallowSite(notification.documents[i]);
>+            }
>+          }

The alignment is weird here, off by a couple spaces.

Otherwise this seems sane with the Gecko changes.
Attachment #350940 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][needs review gavin] → [has patch][needs review bz]
Boris, want to check the changes in DispatchChromeEvent?
Attachment #350940 - Attachment is obsolete: true
Attachment #362190 - Attachment is obsolete: true
Attachment #362977 - Flags: review?(bzbarsky)
Attachment #362190 - Flags: review?(bzbarsky)
Attachment #362977 - Attachment is patch: true
Attachment #362977 - Attachment mime type: application/octet-stream → text/plain
Attachment #362977 - Flags: review?(bzbarsky) → review+
Comment on attachment 362977 [details] [diff] [review]
merged, fixed up DispatchChromeEvent()

>diff -r 53d900594ab5 browser/base/content/browser.js

>+    if (notification) {
>+      notification.documents.push(aContentWindow.document);

Lumping all requests like this doesn't look right to me. Won't this mean that the hostname in the notification bar message will no longer be reflective of what you're actually allowing? An evil site could abuse this by triggering a legitimate site's prompt (from an iframe, say) and then piggybacking along. That may be a contrived example, but seems like we could do better here. Even just always adding a new notification would be better, I think.
(In reply to comment #22)

There's a new notification for each host.
The code doesn't appear to check that the current notification has the same host, it just does getNotificationWithValue("offline-app-requested"). What am I missing?
Whiteboard: [has patch][needs review bz] → [has patch]
Oh. I'm missing the part where this patch changes that to include the host. Nevermind!
http://hg.mozilla.org/mozilla-central/rev/17ff98827b9c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b3414a35b50f
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Comment on attachment 362977 [details] [diff] [review]
merged, fixed up DispatchChromeEvent()

>+nsContentUtils::DispatchChromeEvent(nsIDocument *aDoc,
...
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  rv = aDoc->GetWindow()->GetChromeEventHandler()->DispatchDOMEvent(nsnull,
>+                                                                    event,
>+                                                                    nsnull,
>+                                                                    &status);

Both GetWindow() and GetChromeEventHandler() should be null checked (per the
naming convention where GetXXX methods may return nsnull).

Sorry for not checking this earlier, although I did mentioned about null checks in
comment 14 ;)
Uh.  They are.  At the beginning of the method:

+  NS_ENSURE_ARG_POINTER(aDoc->GetWindow());
+  NS_ENSURE_ARG_POINTER(aDoc->GetWindow()->GetChromeEventHandler());
(In reply to comment #28)
> +  NS_ENSURE_ARG_POINTER(aDoc->GetWindow());

I'm hitting this ~25 times on startup. Does it need to spam the console every time it fails?
Blocks: FF2SM
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: