621158 - DOM Storage quota warning broken with content processes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Josh Matthews [:jdm]]

The quota check happens in the chrome process and it attempts to look at the top of the JS context stack for the window object, which is nonsensical.  This will cause no warnings to be displayed by the browser frontend (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5777).

Comment 1

[:Gijs (he/him)]

Attachment: WIP: make appcache use messaging for quota management, f?mayhemer,mconley

Honza, the test from bug 1093603 doesn't work for at least 3 reasons. 2 are fixed by this patch, namely forwarding the MozApplicationManifest event and keeping the manifestURI available in the parent process so we can find the document that matches a given manifest URI and can give a quota notification. Unfortunately, the third part seems to be busted in core, namely the forwarding of the oncached notification on the content window does not seem to work in e10s. Can you look at why this is broken and how it should be fixed?

Comment 2

[Honza Bambas (:mayhemer)]

Why are you actually bothering with appcache when we are about to remove it soon?

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8724817 [details] [diff] [review]
WIP: make appcache use messaging for quota management, f?mayhemer,mconley

Unless this is critically blocking some more work on Quota Manager (which I presume) I'm not willing to spend time on this.  Appcache is dead.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Comment on attachment 8724817 [details] [diff] [review]
> WIP: make appcache use messaging for quota management, f?mayhemer,mconley
> 
> Unless this is critically blocking some more work on Quota Manager (which I
> presume) I'm not willing to spend time on this.  Appcache is dead.

(which I presume NOT)

Comment 5

[Jim Mathies [:jimm]]

when appcache be removed?

Comment 6

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Why are you actually bothering with appcache when we are about to remove it
> soon?

Probably because appcache being dead and/or it being removed is not a matter of public record (ie: I had no idea). As Jim asked, it would be useful to know when this is getting removed and/or if this means we don't care about it being half-broken in e10s. Should we remove the frontend management code sooner, considering it's badly tested and half-broken in e10s? Where is this being discussed/implemented (ie bug numbers / discussion threads, etc. ) ?

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Jim Mathies [:jimm] from comment #5)
> when appcache be removed?

Please direct these questions to Ehsan.  And I believe this has been posted to dev.platform or dev.planning or blogged about.  Not sure tho.

Comment 8

[Honza Bambas (:mayhemer)]

see bug 1237782.

Comment 9

[Jim Mathies [:jimm]]

I filed bug 1252860 about turning this off with e10s.

Comment 10

[Jim Mathies [:jimm]]

Based on the feedback in bug 1252860, we can't disable or ignore appcache bugs under e10s. renoming.

Gijs, do you have a sense of how much work we have to do here? How badly broken is appcache and its ui?

Comment 11

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] from comment #10)
> Based on the feedback in bug 1252860, we can't disable or ignore appcache
> bugs under e10s. renoming.
> 
> Gijs, do you have a sense of how much work we have to do here? How badly
> broken is appcache and its ui?

I don't know because I don't know enough about appcache. Based on the test, it looks like oncached callbacks are not called as they should be in e10s mode, but I don't know if that issue is specific to the quota case - the quota system is off by default, AIUI, so it might be less of an issue if it's specific to the quota handling in browser UI.

Honza, can you at least investigate if/how/why the oncached thing is broken and if that means there is wider appcache brokenness in e10s? Apparently we can't unship appcache specifically in e10s mode, in which case we should also not ship it half-broken. :-\

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Jim Mathies [:jimm] from comment #10)
> > Based on the feedback in bug 1252860, we can't disable or ignore appcache
> > bugs under e10s. renoming.
> > 
> > Gijs, do you have a sense of how much work we have to do here? How badly
> > broken is appcache and its ui?
> 
> I don't know because I don't know enough about appcache. Based on the test,
> it looks like oncached callbacks are not called as they should be in e10s
> mode, but I don't know if that issue is specific to the quota case - the
> quota system is off by default, AIUI, so it might be less of an issue if
> it's specific to the quota handling in browser UI.
> 
> Honza, can you at least investigate if/how/why the oncached thing is broken
> and if that means there is wider appcache brokenness in e10s? Apparently we
> can't unship appcache specifically in e10s mode, in which case we should
> also not ship it half-broken. :-\

But title says something about quota warning.  Is it related to quota manager or to the quota coming from appcache?

Your comment tho says something about oncached (I assume windows.applicationCache.oncached handler?) not being called on child.

What is this bug then about exactly?  Should we just update the title?

Comment 13

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > (In reply to Jim Mathies [:jimm] from comment #10)
> > > Based on the feedback in bug 1252860, we can't disable or ignore appcache
> > > bugs under e10s. renoming.
> > > 
> > > Gijs, do you have a sense of how much work we have to do here? How badly
> > > broken is appcache and its ui?
> > 
> > I don't know because I don't know enough about appcache. Based on the test,
> > it looks like oncached callbacks are not called as they should be in e10s
> > mode, but I don't know if that issue is specific to the quota case - the
> > quota system is off by default, AIUI, so it might be less of an issue if
> > it's specific to the quota handling in browser UI.
> > 
> > Honza, can you at least investigate if/how/why the oncached thing is broken
> > and if that means there is wider appcache brokenness in e10s? Apparently we
> > can't unship appcache specifically in e10s mode, in which case we should
> > also not ship it half-broken. :-\
> 
> But title says something about quota warning.  Is it related to quota
> manager or to the quota coming from appcache?
> 
> Your comment tho says something about oncached (I assume
> windows.applicationCache.oncached handler?) not being called on child.
> 
> What is this bug then about exactly?  Should we just update the title?

The user-visible bug here is that the quota management UI is broken.

I tried to fix bug 1093603, ie to make the quota manager browser UI test work on e10s, because our lacking e10s test coverage is a cause for concern.

I put up the WIP patch that I have. The test still doesn't work with that patch applied because the oncached handler is not called.

I don't know if that's specific to the quota manager situation, or if oncached only gets called in the quota manager situation and not "normally", or if oncached should be called irrespective of the quota manager situation. Jim asked me how much is broken, and so I said I don't know and I'm asking you to take a look because you actually know something about our appcache implementation, which I do not. So, please could you take a look.

If you think the title of the bug needs changing please make it reflect what you think the reality is - I don't know.

Comment 14

[Honza Bambas (:mayhemer)]

Thanks for the explanation.  Now I follow.  You are trying to fix the quota management UI (the appcache specific one - not related to "Quota Manager") but you are blocked with an issue that windows.applicationCache.oncached callback is not called in e10s.


I did a manual test.  We don't call onchecking and oncached events, 100% reproducible.  I'll file a separate bug and fix it.


Few info: windows.applicationCache.oncached (or shortly oncached) is called after the appcache update process is done for the first time (first caching).  It's not called on the same cache manifest updates.

Comment 15

[Honza Bambas (:mayhemer)]

The problem seems to be on a completely different place. 

From logs I can see that update for the "offlineQuotaNotification.cacheManifest" manifest is scheduled ON THE PARENT only - there is no child side that could ever receive ANY notification.  This is pretty much weird.  

Only after the test is stuck and I manually refresh (F5) the page (offlineQuotaNotification.html) I can see the IPC mechanics are started as expected.

This is beyond my abilities to investigate.  Returning to you.

Comment 16

[Honza Bambas (:mayhemer)]

And note from my point of view and my manual testing appcache works as expected in e10s.  With the minor bug 1253593 fixed it's in parity with single process.

The test here is wrong.  I didn't write it.  I cannot help.

Comment 17

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #15)
> The problem seems to be on a completely different place. 
> 
> From logs I can see that update for the
> "offlineQuotaNotification.cacheManifest" manifest is scheduled ON THE PARENT
> only - there is no child side that could ever receive ANY notification. 
> This is pretty much weird.  

I'm pretty sure this is because the code here (and on mobile, fwiw...) is calling the updating methods in the parent and on the parent window:

https://dxr.mozilla.org/mozilla-central/rev/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/browser/base/content/browser.js#5981

https://dxr.mozilla.org/mozilla-central/rev/b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad/mobile/android/chrome/content/OfflineApps.js#75

Both of those should presumably be passing the content window? Honza, is that right? Or is that window reference supposed to be a chrome-level parent window or something like that? (If you don't know, can you suggest someone else who *would* know? :-) )

Comment 18

[Honza Bambas (:mayhemer)]

Sorry, you simply have to go through annotations/blame.  I don't know who is responsible for this piece of code and how the failing test works.

Comment 19

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #18)
> Sorry, you simply have to go through annotations/blame.  I don't know who is
> responsible for this piece of code and how the failing test works.

Please reread comment #17. I'm asking about the scheduleUpdate method's window parameter. You added that parameter: https://hg.mozilla.org/mozilla-central/rev/c73c0da830fe .

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to :Gijs Kruitbosch from comment #19)
> (In reply to Honza Bambas (:mayhemer) from comment #18)
> > Sorry, you simply have to go through annotations/blame.  I don't know who is
> > responsible for this piece of code and how the failing test works.
> 
> Please reread comment #17. I'm asking about the scheduleUpdate method's
> window parameter. You added that parameter:
> https://hg.mozilla.org/mozilla-central/rev/c73c0da830fe .

Sorry, wasn't clear what info you needed.  Still I really don't know how to help you.  I didn't write the original code of browser/base/content/browser.js.  The window arg (I don't care if it's correct or not, that is up to the caller code to provide the correct window - out of my control) is passed down to get the appropriate tabchild to start the appcache update processing on the parent (do the proper IPC calls).

I don't know if browser/base/content/browser.js is a parent thing or a child thing and where from this particular piece if code that I have modified is called from.

Your objective now is to find out where from - during the failing test - the appcache update is scheduled (as I observed, it was happening only and only on the parent) and why it's not scheduled from the child process via child instance of the offline cache update service.  This is something I really cannot help you with.  I've only provided necessary IPC child side implementation, but I'm not responsible for properly invoking it.  It was completely transparent to me, because offline cache updates are (with e10s on) scheduled from nsContentSink that lives only on child.  Callers of offline appcache update invocation have not been modified by c73c0da830fe except adding the arg and few small tweaks since child cannot access some offline appcache properties directly (like foreign marking, as one example).

The failing test is IMO badly written.

Only help I can gladly provide is an explanation of how the appcache updating invocation works (or is expected to work on e10s).  That can make some steps easier for you.  I know, appcache is a bitch.  I can't wait to remove it from all around the platform!

Comment 21

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> (In reply to :Gijs Kruitbosch from comment #19)
> > (In reply to Honza Bambas (:mayhemer) from comment #18)
> > > Sorry, you simply have to go through annotations/blame.  I don't know who is
> > > responsible for this piece of code and how the failing test works.
> > 
> > Please reread comment #17. I'm asking about the scheduleUpdate method's
> > window parameter. You added that parameter:
> > https://hg.mozilla.org/mozilla-central/rev/c73c0da830fe .
> 
> Sorry, wasn't clear what info you needed.  Still I really don't know how to
> help you.  I didn't write the original code of
> browser/base/content/browser.js.  The window arg (I don't care if it's
> correct or not

Err, what?

> , that is up to the caller code to provide the correct window
> - out of my control)

Um, no, you pass |window|. That's not an argument from the caller, it's just a global reference to the window in which browser/base/content/browser.js runs, ie the window running browser.xul.

browser/base/content/browser.js runs in the parent process. You were the one who added the window param, and passed it here - but it seems like that will just never work in e10s because it's a reference to the XUL browser window, not to the window containing the document in the content, and of necessity, in e10s, it can *never* be a content window, because the code runs in the parent process so it's not possible to have a reference to a content process window in the first place. As in, there is no correct parameter that could possibly work here.

Is there an alternative API we can use from within the parent process, or do we need to proxy this to the child process?

> is passed down to get the appropriate tabchild to start
> the appcache update processing on the parent (do the proper IPC calls).

OK, so you're saying that the browser code that wants to schedule this update should be making this call in the content process?

> Your objective now is to find out where from - during the failing test - the
> appcache update is scheduled (as I observed, it was happening only and only
> on the parent)

Of necessity, the only callers are in the parent process on the chrome (not the test!) side. This is just broken - the test isn't broken, the code that the test is testing is broken, and you wrote that code.

Do you have time to fix that code? As we're trying to push e10s out of the door, we need to make sure it behaves no worse than non-e10s. I investigated the test and fixed several issues already in the WIP patch that I attached, but it seems this is another issue that needs addressing.

> Only help I can gladly provide is an explanation of how the appcache
> updating invocation works (or is expected to work on e10s).  That can make
> some steps easier for you.

Right, so, if you really don't have time to work on this, please do explain how you expect this code to work on e10s.

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to :Gijs Kruitbosch from comment #21)
> Um, no, you pass |window|. That's not an argument from the caller, it's just
> a global reference to the window in which browser/base/content/browser.js
> runs, ie the window running browser.xul.

In non-e10s this argument is ignored.

And as we don't have e10s tests, this is broken forever.  Interesting this went through the review.  I took the window here for granted in believe it's caught by some test (automated or manual).

> 
> browser/base/content/browser.js runs in the parent process. You were the one
> who added the window param, and passed it here - but it seems like that will
> just never work in e10s because it's a reference to the XUL browser window,
> not to the window containing the document in the content, and of necessity,
> in e10s, it can *never* be a content window, because the code runs in the
> parent process so it's not possible to have a reference to a content process
> window in the first place. As in, there is no correct parameter that could
> possibly work here.

Aha!  Now we are getting somewhere.  So, this code (which as you claim below is faulty!) is ran after user allows the appcache be installed manually, right?  It means, on the parent of course.

This way you never get a notification on the child, as I will explain below.

> 
> Is there an alternative API we can use from within the parent process, or do
> we need to proxy this to the child process?

No API.  We will have to provide it or you need to proxy this to the child.  See also below.

> 
> > is passed down to get the appropriate tabchild to start
> > the appcache update processing on the parent (do the proper IPC calls).
> 
> OK, so you're saying that the browser code that wants to schedule this
> update should be making this call in the content process?

Simply said, but keep reading please.
 
> > Your objective now is to find out where from - during the failing test - the
> > appcache update is scheduled (as I observed, it was happening only and only
> > on the parent)
> 
> Of necessity, the only callers are in the parent process on the chrome (not
> the test!) side. This is just broken - the test isn't broken, the code that
> the test is testing is broken, and you wrote that code.

I agree.  The thing we never noticed is probably because we allow appcache be downloaded (cached) automatically.  There is no prompt and only a handful of appcaches in the wild goes over the quota limit to ask for permissions from user before the update process is started.

> 
> Do you have time to fix that code? 

Definitely not, but I can assist with advises and reviews.

> As we're trying to push e10s out of the
> door, we need to make sure it behaves no worse than non-e10s. I investigated
> the test and fixed several issues already in the WIP patch that I attached,
> but it seems this is another issue that needs addressing.
> 
> > Only help I can gladly provide is an explanation of how the appcache
> > updating invocation works (or is expected to work on e10s).  That can make
> > some steps easier for you.
> 
> Right, so, if you really don't have time to work on this, please do explain
> how you expect this code to work on e10s.

Under usual circumstances when appcache fits the (I think) 5MB limit the following happens with e10s ON when a page with appcache manifest is hit THE FIRST TIME (where you expect to get the oncached notification when its done):

- we are starting on the child process:
- the HTML parser hits the <html manifest=".."> attribute
- it calls nsContentSink::ProcessOfflineManifest
- it schedules (via the nsIOfflineCacheUpdate.scheduleOnDocumentStop) an update to be started after the document is loaded
- after the document is loaded OfflineCacheUpdateChild is started
- OfflineCacheUpdateChild is added an observer, which is the window.applicationCache object (=nsDOMOfflineResourceList - yeah, a crazy name) to receive all the on* notifications from the update
  - the window is NOT the one passed to any schedule* method ; nsDOMOfflineResourceList adds itself because it's notified an "offline-cache-update-added" global notification and the manifest URL of the update being added and the one of the nsDOMOfflineResourceList matches
- OfflineCacheUpdateChild is given a DOM window that is queried for a tab-child (via the schedule method)
- OfflineCacheUpdateChild then IPCs the tab-child to start the update process on the parent
- now on the parent process:
- OfflineCacheUpdateParent and nsOfflineCacheUpdate instances are started
- those are responsible for the offline cache update processing itself: download the manifest, resources, commit the cache and, most importantly for you, provide the notifications
- the notifications are IPC'ed to the OfflineCacheUpdateChild that forwards them to all its added observers (it's the nsDOMOfflineResourceList instance which is the window.applicationCache object)

That's it.  But this chain of events happens only when the update is scheduled from the child side.

So, the faulty code should call offlineCacheUpdateService.schedule*() on the child or you can add a new method to the service to start the update from the child side.

Comment 23

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #22)
> > Do you have time to fix that code? 
> 
> Definitely not, but I can assist with advises and reviews.

OK. I'll have a look hopefully later this week.

Comment 24

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 621158 - make appcache use messaging for quota management, r?mayhemer,jaws

Review commit: https://reviewboard.mozilla.org/r/39723/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39723/

Comment 25

[:Gijs (he/him)]

Comment on attachment 8730157 [details]
MozReview Request: Bug 621158 - make appcache use messaging for quota management, r?mayhemer,jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39723/diff/1-2/

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: rb39723.patch (since I hate mozreview)

Comment 27

[Honza Bambas (:mayhemer)]

Can you please describe what the patch does?  That would simplify the review.  Thanks.

Comment 28

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #27)
> Can you please describe what the patch does?  That would simplify the
> review.  Thanks.

It moves the MozApplicationManifest listener into the content process. If appcache is not allowed by default, and hasn't been specifically allowed on that domain (all checked in the content process) we send a message to the parent process to prompt the user. If the user denies the access, we tell the permission manager this in the parent process. If the user allows the access, we tell the permission manager this in the parent process, and we send a message back to the content process to schedule the update there. In order to ensure we do this on the right document and window, the original message to the parent process has a (per-tab-unique) document ID that maps to a weak ref to a document. If the document is still alive, we schedule an update on it in the content process.

If, when a MozApplicationManifest listener sends its message to the parent process, a notification for that host already exists, we add the set (browser, [document] uri, document ID) to the list of things that need to be notified if the user allows/denies the request.


Separately, if a MozApplicationManifest is triggered, even if appcache is allowed by default, we add a weak observer to the observer service for that tab, watching for "offline-cache-update-completed", and add the manifest URI we've observed to a set of "seen" manifest URIs. If an offline-cache-update-completed notification is sent by the observer service with a cacheUpdate as subject that has a manifestURI in that set, we notify the parent process. The parent process then checks usage for that URI, and if necessary shows a warning notification to the user. If a notification is shown, we also tell the permission manager so we won't warn the user again.

Note that this means that we'll notify the user even if the document that caused the update to happen has been unloaded at that point, unless the tab itself is closed and the script (and observer) that's listening on that tab is destroyed by then, in which case we'll warn the next time when the user opens a tab that triggers that notification. Because the notification doesn't have a way to link the notification to a window/document/browser directly, I think this is the best we can do without a bigger rearchitecturing that does not seem warranted given appcache's status, and additionally not really a good idea considering this is marked for e10s m9 and we probably will want to look at uplifting this.


Does this help?

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8730157 [details]
MozReview Request: Bug 621158 - make appcache use messaging for quota management, r?mayhemer,jaws

https://reviewboard.mozilla.org/r/39723/#review37295

Preliminary feedback, still not done with the review.

::: browser/base/content/browser.js:5764
(Diff revision 2)
>    // XXX: duplicated in preferences/advanced.js
> -  _getOfflineAppUsage: function (host, groups)
> -  {
> +  _getOfflineAppUsage(host) {
> +    let cacheService = Cc["@mozilla.org/network/application-cache-service;1"].
> -    var cacheService = Cc["@mozilla.org/network/application-cache-service;1"].
>                         getService(Ci.nsIApplicationCacheService);
> -    if (!groups)
> -      groups = cacheService.getGroups();
> +    let groups = cacheService.getGroups();
> +    let usage = 0;
> -
> -    var usage = 0;
>      for (let group of groups) {
> -      var uri = Services.io.newURI(group, null, null);
> +      let uri = Services.io.newURI(group, null, null);
>        if (uri.asciiHost == host) {
> -        var cache = cacheService.getActiveCache(group);
> +        let cache = cacheService.getActiveCache(group);
>          usage += cache.usage;
>        }
>      }
>  
>      return usage;
>    },

The comment here says that this function is duplicated in preferences/advanced.js.

Note that these are out of sync now, as the one in advanced.js uses a permission object instead of the host parameter. browser.js should be updated to use a permission object as well.

Further, this patch now makes these two functions even more different. Can you update the advanced.js implementation to include these changes? As well as file a bug to have browser.js use a permission object?

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 8730157 [details]
MozReview Request: Bug 621158 - make appcache use messaging for quota management, r?mayhemer,jaws

https://reviewboard.mozilla.org/r/39723/#review37573

The hard part for me here is that I'm not at all familiar with the browser code, remote browser even more.  I'm r+'ing this patch based on your description that seems reasonable.  I haven't tested this patch manually, leaving it in your discretion.

A browser peer MUST review this code as well.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8730157 [details]
MozReview Request: Bug 621158 - make appcache use messaging for quota management, r?mayhemer,jaws

https://reviewboard.mozilla.org/r/39723/#review37699

::: browser/base/content/browser.js:5752
(Diff revision 2)
>      };
>  
> -    let warnQuota = gPrefService.getIntPref("offline-apps.quota.warn");
> +    let warnQuota = Services.prefs.getIntPref("offline-apps.quota.warn");
>      let message = gNavigatorBundle.getFormattedString("offlineApps.usage",
> -                                                      [ aURI.host,
> +                                                      [ uri.host,
>                                                          warnQuota / 1024 ]);

Why do we divide offline-apps.quota.warn by 1024 here, but in checkUsage we mulitply it by 1024?

::: browser/base/content/browser.js:5861
(Diff revision 2)
> -    var manifest = aDocument.documentElement.getAttribute("manifest");
> -    if (!manifest)
> +        if (this._checkUsage(uri)) {
> +          this.warnUsage(msg.target, uri);

The naming of these two together is a bit confusing. checkUsage returns true if the usage is over the default limit, but the name makes it seem that it will return true if it's "used". Please rename checkUsage to usedMoreThanWarnQuota.

::: browser/base/content/content.js:1338
(Diff revision 2)
> +    }
> +    let manifestURI = this._getManifestURI(aWindow);
> +    this._docManifestSet.add(manifestURI.spec);
> +  },
> +
> +  handleEvent: function(event) {

nit, handleEvent(event) {

::: browser/base/content/content.js:1344
(Diff revision 2)
> +    if (event.type == "MozApplicationManifest") {
> +      this.offlineAppRequested(event.originalTarget.defaultView);
> +    }
> +  },
> +
> +  _getManifestURI: function(aWindow) {

nit, _getManifestURI(aWindow) {

::: browser/base/content/content.js:1360
(Diff revision 2)
> +    } catch (e) {
> +      return null;
> +    }
> +  },
> +
> +  offlineAppRequested: function(aContentWindow) {

nit, offlineAppRequested(aContentWindow) {

::: browser/base/content/test/general/browser_offlineQuotaNotification.js:53
(Diff revision 2)
> +    let onCachedAttached = new Promise(resolve => {
> +      let onCachedAttached = msg => {
> +        mm.removeMessageListener("Test:OnCachedAttached", onCachedAttached);
> +        resolve();
> +      };
> +      let mm = gBrowser.selectedBrowser.messageManager;
> +      mm.addMessageListener("Test:OnCachedAttached", onCachedAttached);
> +    });

Please replace this with BrowserTestUtils.waitForMessage

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c95124453340

Comment 33

[:Gijs (he/him)]

(In reply to Pulsebot from comment #32)
> https://hg.mozilla.org/integration/fx-team/rev/c95124453340

... annnnnd out again:

remote:   https://hg.mozilla.org/integration/fx-team/rev/ad73190dcc49


for breaking mochitest-1:

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=c95124453340&selectedJob=8122238

 10 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/test_offline_gzip.html | uncaught exception - TypeError: panel.firstElementChild is null at loaded@http://mochi.test:8888/tests/browser/base/content/test/general/test_offline_gzip.html:109:3 

I'm guessing this test is assuming things are sync, and now they no longer are. :-\

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8279c04272ee

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7dfa1b5d2607

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8279c04272ee
https://hg.mozilla.org/mozilla-central/rev/7dfa1b5d2607

Comment 38

[:Gijs (he/him)]

Attachment: Patch for aurora

Comment 39

[:Gijs (he/him)]

Comment on attachment 8737228 [details] [diff] [review]
Patch for aurora

This is a combination of the csets that landed in this bug and bug 1257488.

Approval Request Comment
[Feature/regressing bug #]: appcache quotas, e10s
[User impact if declined]: appcache quotas don't work in e10s mode
[Describe test coverage new/current, TreeHerder]: this enables automated tests
[Risks and why]: low-ish. It's early enough in aurora that this is fairly safe. The alternative is that it for sure doesn't work, so it's hard to do worse...
[String/UUID change made/needed]: nope.

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8737228 [details] [diff] [review]
Patch for aurora

Appcache related fixes needed in e10s mode, Aurora47+

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80d2aa7ab529