Closed
Bug 1129662
Opened 6 years ago
Closed 5 years ago
[e10s] sdk/page-worker should use a remote page
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Add-on SDK Graveyard
General
Tracking
(e10s+, firefox45 fixed)
RESOLVED
FIXED
mozilla45
People
(Reporter: mossop, Assigned: mossop)
References
Details
Attachments
(1 file, 1 obsolete file)
When in e10s mode page-worker should load its page remotely.
Assignee | ||
Updated•6 years ago
|
tracking-e10s:
--- → +
Updated•6 years ago
|
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Comment 1•6 years ago
|
||
Hey Dave, I'm not sure what change you are suggesting here, could you describe it in more detail?
Flags: needinfo?(dtownsend)
Assignee | ||
Comment 2•6 years ago
|
||
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.
Flags: needinfo?(dtownsend)
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → dtownsend
Assignee | ||
Comment 3•5 years ago
|
||
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?
Flags: needinfo?(gkrizsanits)
Comment 4•5 years ago
|
||
(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...
Flags: needinfo?(gkrizsanits)
Comment 5•5 years ago
|
||
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
Assignee | ||
Comment 6•5 years ago
|
||
(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•5 years ago
|
||
(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 :(
Assignee | ||
Comment 8•5 years ago
|
||
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•5 years ago
|
||
(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)
Assignee | ||
Comment 10•5 years ago
|
||
(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.
Assignee | ||
Comment 11•5 years ago
|
||
(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•5 years ago
|
||
(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 :)
Assignee | ||
Comment 13•5 years ago
|
||
I figured out the problem with content-document-global-created is bug 1129662. Making some progress with the web progress listener though.
Assignee | ||
Comment 14•5 years ago
|
||
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.
Attachment #8679197 -
Flags: review?(gkrizsanits)
Assignee | ||
Updated•5 years ago
|
Attachment #8675846 -
Attachment is obsolete: true
Comment 15•5 years ago
|
||
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.
Attachment #8679197 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 16•5 years ago
|
||
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.
Attachment #8679197 -
Attachment description: MozReview Request: Bug 1129662: sdk/page-worker should use a remote page. r?gabor → MozReview Request: Bug 1129662: sdk/page-worker should use a remote page. r?krizsa
Attachment #8679197 -
Flags: review?(gkrizsanits)
Comment 17•5 years ago
|
||
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
Attachment #8679197 -
Flags: review?(gkrizsanits) → review+
Comment 19•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5cdf66dfef92
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 20•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5cdf66dfef92
status-b2g-v2.5:
--- → fixed
Comment 21•5 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5:
fixed → ---
Comment 22•5 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•