1129662 - [e10s] sdk/page-worker should use a remote page

Status: RESOLVED FIXED

Description

[Dave Townsend [:mossop] (he/him)]

When in e10s mode page-worker should load its page remotely.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Hey Dave, I'm not sure what change you are suggesting here, could you describe it in more detail?

Comment 2

[Dave Townsend [:mossop] (he/him)]

page-worker should load its pages in the content process rather than the main process to keep web content separated from the chrome. At a basic level it should just be switching page-worker to load in a <browser remote="true">, but chances are there will be test and other changes related to getting that working.

Comment 3

[Dave Townsend [:mossop] (he/him)]

Gabor I've got this mostly working but I'm hitting a strange thing with about:blank documents loaded in a hidden docshell. For some reason I don't get document-element-inserted observer notifications for these, do you know anything about that?

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #3)
> Gabor I've got this mostly working but I'm hitting a strange thing with
> about:blank documents loaded in a hidden docshell. For some reason I don't
> get document-element-inserted observer notifications for these, do you know
> anything about that?

Not sure about hidden docshell, I would expect it to work quite similarly to the regular docshells.
Which is weird in itself. If a content document is loaded then we load first an about:blank page in the content process and fire a document-element-inserted for it there, if we load a chrome page then we load an about:blank document on chrome side and fire the even there. Now the confusin bit is that if we load an about:blank page directly then the even is not fired at all... This whole notification is quite unconsistent, and should be reimplemented imo, I just talked about this with smaug on IRC. Anyway you should check that you are trying to observe it in the right process, but I'm affraid you already do, and your case is the last one. Any workaround we can do? What is the use case exactly? I can try to do some work on platform side to get the behavior what we need if we cannot find a workaround...

Comment 5

[Gabor Krizsanits (INACTIVE)]

As a side node we also don't seem to fire the notification for data document like: "data:text/html"

I think it makes sense to file a bug about this: Bug 1216159

Comment 6

[Dave Townsend [:mossop] (he/him)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> (In reply to Dave Townsend [:mossop] from comment #3)
> > Gabor I've got this mostly working but I'm hitting a strange thing with
> > about:blank documents loaded in a hidden docshell. For some reason I don't
> > get document-element-inserted observer notifications for these, do you know
> > anything about that?
> 
> Not sure about hidden docshell, I would expect it to work quite similarly to
> the regular docshells.
> Which is weird in itself. If a content document is loaded then we load first
> an about:blank page in the content process and fire a
> document-element-inserted for it there, if we load a chrome page then we
> load an about:blank document on chrome side and fire the even there. Now the
> confusin bit is that if we load an about:blank page directly then the even
> is not fired at all... This whole notification is quite unconsistent, and
> should be reimplemented imo, I just talked about this with smaug on IRC.
> Anyway you should check that you are trying to observe it in the right
> process, but I'm affraid you already do, and your case is the last one. Any
> workaround we can do? What is the use case exactly? I can try to do some
> work on platform side to get the behavior what we need if we cannot find a
> workaround...

Yeah it sounds like the last case is what is happening. I'm creating my hidden docshell in the child process and listening for notifications there so I wouldn't expect anything to be going on in the parent process. The use case is that page-worker waits for a page to start loading before attaching a worker to it and in some of the tests (so maybe some real add-ons) we load about:blank either as the first page or sometimes switching from some other page to about:blank. My listener never sees that load and so never attaches the worker. I might be able to do some workaround with the content-document-global-created notification but I think I was seeing inconsistent results there too.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #6)
> case is that page-worker waits for a page to start loading before attaching
> a worker to it and in some of the tests (so maybe some real add-ons) we load
> about:blank either as the first page or sometimes switching from some other
> page to about:blank. My listener never sees that load and so never attaches

It's weird that this used to work, I wonder why... I think I should fix Bug 1216159,
triggering the notification consistently would be a good thing after all. Could you
file a patch with I can reproduce the failures you see? (an unpolished WIP patch
is totally fine)

> the worker. I might be able to do some workaround with the
> content-document-global-created notification but I think I was seeing
> inconsistent results there too.

Too bad :(

Comment 8

[Dave Townsend [:mossop] (he/him)]

Attachment: patch

Here is my current work. I think it's almost done but it is failing in test-page-worker.js in testRedirectIncludeArrays where the worker script redirects the page to about:blank and the page-worker code never sees document-element-inserted for that. There is some logging in there too that should show that.

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #8)
> Created attachment 8675846 [details] [diff] [review]
> patch
> 
> Here is my current work. I think it's almost done but it is failing in
> test-page-worker.js in testRedirectIncludeArrays where the worker script
> redirects the page to about:blank and the page-worker code never sees
> document-element-inserted for that. There is some logging in there too that
> should show that.

Thanks Dave, this looks great. I looked into this and it seems like that document-element-inserted has actually never been triggered in that test. The test relies on the document load event. Which is the default if the 'when' attribute is not set. And I think the old version is listening for this event on the add-on window, since we used to load pages in the hidden window. Anyway I think the observer you use should handle the "start" case only. And in the ChildPage we should listen for the DOMContentLoaded and the load event for the document, and handle the "end" and "ready" cases there. That would give us identical behavior with the old version (or so I hope...). For attaching listeners like that you might need an nsIWebProgressListener like http://mxr.mozilla.org/mozilla-central/source/docshell/test/chrome/test_bug565388.xul#23 and then get the DOMWindow from the nsIWebProgress when the request has stopped. What do you think?

I looked into the document-element-inserted bug, but I'm afraid that will be a tricky to land bug, and I would not like to be depending on it if it's avoidable. (I still think that's something to be fixed but I'm quite positive it will break things and will take time to get it right and land)

Comment 10

[Dave Townsend [:mossop] (he/him)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> (In reply to Dave Townsend [:mossop] from comment #8)
> > Created attachment 8675846 [details] [diff] [review]
> > patch
> > 
> > Here is my current work. I think it's almost done but it is failing in
> > test-page-worker.js in testRedirectIncludeArrays where the worker script
> > redirects the page to about:blank and the page-worker code never sees
> > document-element-inserted for that. There is some logging in there too that
> > should show that.
> 
> Thanks Dave, this looks great. I looked into this and it seems like that
> document-element-inserted has actually never been triggered in that test.
> The test relies on the document load event. Which is the default if the
> 'when' attribute is not set. And I think the old version is listening for
> this event on the add-on window, since we used to load pages in the hidden
> window. Anyway I think the observer you use should handle the "start" case
> only. And in the ChildPage we should listen for the DOMContentLoaded and the
> load event for the document, and handle the "end" and "ready" cases there.
> That would give us identical behavior with the old version (or so I
> hope...).

Yes your summary is correct, the original code probably never handled using "start" as a contentScriptWhen for about:blank documents correctly. The problem is the original code handles waiting for the ready and end cases by using a DOM event listener on the hidden window relying on events bubbling up from the frames. In the new code I'm using hidden docshells in the content process instead of individual frames so there is nowhere for those events to bubble up to. So I need to know when a document is available for me to attach listeners to, I'm not sure that nsIWebProgressListener gives me that does it?

The alternative is that I fall back to creating a remote frame for event page-worker instance and then the frame script could wait for the DOM events instead I guess. It just means we'll still be broken for some cases.

Comment 11

[Dave Townsend [:mossop] (he/him)]

(In reply to Dave Townsend [:mossop] from comment #10)
> The alternative is that I fall back to creating a remote frame for event
> page-worker instance and then the frame script could wait for the DOM events
> instead I guess. It just means we'll still be broken for some cases.

That should be "creating a remote frame for every page-worker instance"

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #10)
> events to bubble up to. So I need to know when a document is available for
> me to attach listeners to, I'm not sure that nsIWebProgressListener gives me
> that does it?

Yes, I think that is the only sane way to get it actually. I learned this while I was
writing test for this windowless docshell, the test I linked into my previous comment does pretty much like that (where the test finishes is where you should attach the listeners as far as I know). You can attach a web progress listener to the docshell, start the navigation and when you get STATE_STOP then you should be good to attach the load listener to the document. (you probably have to check the URL as well, you might get a call for about:blank first I'm not sure, it was a long time ago)

I'm not an expert of this API so I think it's more or less trying it out and see if it works. There is some documentation about it (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebProgressListener) and some tests are using it.

> 
> The alternative is that I fall back to creating a remote frame for event
> page-worker instance and then the frame script could wait for the DOM events
> instead I guess. It just means we'll still be broken for some cases.

I'm not against this approach either if it works. It is far better than what we have right now in the tree :)

Comment 13

[Dave Townsend [:mossop] (he/him)]

I figured out the problem with content-document-global-created is bug 1129662. Making some progress with the web progress listener though.

Comment 14

[Dave Townsend [:mossop] (he/him)]

Attachment: MozReview Request: Bug 1129662: sdk/page-worker should use a remote page. r?krizsa

Bug 1129662: sdk/page-worker should use a remote page. r?gabor

This makes page-worker load its pages in the remote process. It does so by
creating a single frame in the hidden window used to ensure we have a remote
process when necessary and then a module in the remote process is used to
create windowless browsers to load the pages.

This does break one API, getActiveView, but I don't think we should be
maintaining that and it has been unstable since its inception anyway.

Once downside, the l10n module now has to use the observer service to detect
documents rather than the DOM event, this might be causing more CPOW traffic
since that observer notification is shimmed so we may need to use the shim
waiver there.

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8679197 [details]
MozReview Request: Bug 1129662: sdk/page-worker should use a remote page. r?krizsa

https://reviewboard.mozilla.org/r/23395/#review20993

This looks really great in general, there is only one thing I'm really not sure about:

::: addon-sdk/source/lib/sdk/content/page-worker.js:59
(Diff revision 1)
> +  onNewWindow: function(window) {
> +    let event = getAttachEventType(this.options);
> +    if (event == "document-element-inserted") {
> +      this.attachWorker();
> +    }

I'm a bit concerned about this part. If I get it right this comes with a behavior change for the "start" case, since the locationchange should be fired sooner than the document-element-inserted event. Actually I wonder if this even works, since at that point I'm not sure that we are ready to execute scripts... do we have a test for that?

For "ready" and "end" this is great but I think we should keep relying on an observer being notified about document-element-inserted for the "start" case. I know that for about:blank that will not work but I assume it never had, so we don't break anything, and in the regular cases it would work exactly as before.

::: addon-sdk/source/lib/sdk/content/page-worker.js:109
(Diff revision 1)
> +  onLocationChange: function(progress, request, location, flags) {
> +    if (progress != this.webProgress)
> +      return;
> +    this.onNewWindow();
> +  },

There are two flags that can be interesting here:
LOCATION_CHANGE_ERROR_PAGE but I think isValidURL should handle this... the other one is LOCATION_CHANGE_SAME_DOCUMENT and I think we should ignore the event if this flag is set, so we can avoid the content script to be attached multiple times, accidentally.

Comment 16

[Dave Townsend [:mossop] (he/him)]

Comment on attachment 8679197 [details]
MozReview Request: Bug 1129662: sdk/page-worker should use a remote page. r?krizsa

Bug 1129662: sdk/page-worker should use a remote page. r?krizsa

This makes page-worker load its pages in the remote process. It does so by
creating a single frame in the hidden window used to ensure we have a remote
process when necessary and then a module in the remote process is used to
create windowless browsers to load the pages.

This does break one API, getActiveView, but I don't think we should be
maintaining that and it has been unstable since its inception anyway.

Once downside, the l10n module now has to use the observer service to detect
documents rather than the DOM event, this might be causing more CPOW traffic
since that observer notification is shimmed so we may need to use the shim
waiver there.

Comment 17

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8679197 [details]
MozReview Request: Bug 1129662: sdk/page-worker should use a remote page. r?krizsa

https://reviewboard.mozilla.org/r/23395/#review21127

Comment 18

[Pulsebot]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cdf66dfef92

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5cdf66dfef92

Comment 21

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 22

[noitidart]

Users are suspecting that this change is causing pages loaded in page-worker to go to history.

Thanks @litho for this bug link - http://forums.mozillazine.org/viewtopic.php?f=19&t=2999033