753981 - XHR in Web Workers bypasses Offline AppCache

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Step to reproduce: write a test case!

Expect result: 
  Data from xhr.responseText, after when readyState = 4.

Actual result:
  xhr.responseText is a 0 byte string.


Use case:
  In B2G Gaia keyboard, I would like to load the dictionary data from a spell check library that I run in the Worker scope.

Workaround:
  Loaded it with XHR of the browser window scope and postMessage the data (haven't get it working for now). Sigh.

Comment 1

[Honza Bambas (:mayhemer)]

I'll take a look.

Comment 2

[Honza Bambas (:mayhemer)]

Tim, I need some test case.  W/o it I cannot move forward, sorry.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Tim, I need some test case.  W/o it I cannot move forward, sorry.

That's all right, it should be my responsibility anyway.

Since the situation requires resources to be available in offline cache but NOT in the http disk cache, I cannot put the scenario into one set of web pages.

If I write a page that is close enough for the scenario and some quick STR in FIrefox (like, say, clean up http cache and turn on offline mode), it's that good enough for you?

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Here you go, STR is the same as bug 748240.

1. Go to http://beta.timc.idv.tw/bug753981/ , make sure it's cached
2. Verified it is AppCache'd by checking about:cache?device=offline
3. Delete the HTTP cache (Preference -> Advanced -> Network -> Cached Web Content -> Clear Now)
4. Go offline (File -> Work Offline)
5. Go back to the website, it should load from AppCache

Expected result:

The last line of textare should be "worker: xhr responseText: Hello World!"

Actual result:

The last line of textare is "worker: xhr responseText: "
Also the readyState 3 is being skipped.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

FYI, one can verified that data.txt is indeed cached by looking for it in about:cache?device=offline

Comment 6

[Honza Bambas (:mayhemer)]

Problem is that in nsXMLHttpRequest::GetLoadGroup() GetContextForEventHandlers() returns NULL.  This prevents to set a load group that is a source for binding the proper app cache with the underlying channel.  nsXMLHttpRequest in a worker doesn't have mOwner.

Is there some way we can reach docshell of the original window that opened the worker?  

Or can the application cache object (if present) be carried from the window to the worker somehow?

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I wonder if this broke with smaug's weak event target stuff...

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Why is the owner null? What is the value of mHasOrHasHadOwner?
XHR sure should have the owner window.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to ben turner [:bent] from comment #7)
> I wonder if this broke with smaug's weak event target stuff...

I don't understand how. If the main page is still alive, XHR behavior should be the same as
before.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Why is the owner null? 

In nsXMLHttpRequest::Init() aOwnerWindow->GetCurrentInnerWindow() returns null.

> What is the value of mHasOrHasHadOwner?
> XHR sure should have the owner window.

false, obviously.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Sounds like a bug in workers then.

Comment 12

[Honza Bambas (:mayhemer)]

See comment 10.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: testcase

In case I delete the test on my site accidentialy...

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Uploaded to people.moz also.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, the problem is that workers hold onto an inner window, and XHR expects an outer window there (without asserting that...). The fix is simple I think.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

I think this should do it.

Try run: https://hg.mozilla.org/try/rev/e9a5897468d3

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ben, is there anything preventing this from landing?

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oops! No!

https://hg.mozilla.org/integration/mozilla-inbound/rev/728b0b12eb7f

Comment 19

[Jason Duell]

aurora-bound?

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jason Duell (:jduell) from comment #19)
> aurora-bound?

Gaia (now deployed as packaged apps) doesn't rely on this to work in any way; but other AppCache'd hosted apps might.

It would be nice if this can be fast-forward in to Aurora, etc.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/728b0b12eb7f

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Blocks a blocker blocker.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Where does this need to land? Tell me and I'll take care of it.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Just b2g18.  It made it into the last aurora uplift.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/56105e051770