STR: 1. open http://mozilla.github.io/pdf.js/web/compressed.tracemonkey-pldi-09.pdf 2. press ctrl/cmd-f 3. type "SELF" expected: find SELF text actual: text SELF was not found

http://www.mathies.com/mozilla/pdf-test.pdf Works on this test case for various words, although the highlight looks like its the wrong color? http://mozilla.github.io/pdf.js/web/compressed.tracemonkey-pldi-09.pdf I didn't find SELF here, but it works for other words, like 'dynamic', 'intel', etc..

testing on windows btw, were you on a different platform?

> Works on this test case for various words, although the highlight looks like its the wrong color? This is a sign of the standard HTML page functionality, which shall be disabled by PDF.js find bar integration. (That means only visible pages text layer can be "searched".) The PDF.js draws its own highlight -- and it works in non-e10s mode.

One thing I noticed, an error in the browser console - TypeError: chromeWindow.gBrowser is undefined PdfStreamConverter.jsm:738 http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#738

I looked this over today while thinking about the exposedProps usage. I think we should just fix this bug by eliminating the uses of CPOWs and just using messages to pass the find events around. It doesn't seem like it would be too hard.

I've tested this today on linux and windows, and both version worked fine with e10s on. As much as I can tell it's the PDF.js integrated search bar what I'm testing (at the top of the page in the PDF.js UI bar as opposed to the standard HTML page search at the bottom of the screen which is now disabled) Could you please check this again and confirm that this bug is indeed gone?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6) > I've tested this today on linux and windows, and both version worked fine > with e10s on. As much as I can tell it's the PDF.js integrated search bar > what I'm testing (at the top of the page in the PDF.js UI bar as opposed to > the standard HTML page search at the bottom of the screen which is now > disabled) > > Could you please check this again and confirm that this bug is indeed gone? It is not gone. And it got worse. It does not show integrated find anymore -- compare with non-e10s. New STR to avoid keyboard usage: 1. open http://mozilla.github.io/pdf.js/web/compressed.tracemonkey-pldi-09.pdf#page=1 2. open find bar using menu Edit->Find 3. type "SELF"

(In reply to Yury Delendik (:yury) from comment #7) > It is not gone. And it got worse. Right, I think I mixed up the good case with the bad case, sorry for the confusion and wasting your time. (In reply to Bill McCloskey (:billm) from comment #5) > I looked this over today while thinking about the exposedProps usage. I > think we should just fix this bug by eliminating the uses of CPOWs and just > using messages to pass the find events around. It doesn't seem like it would > be too hard. Yes, this is the issue it seems. (http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm#300) So I could probably do what you suggest, but I'm afraid that we have a more generic issue here to fix. Namely, we do have an API for exactly this, exposing chrome functions to content in a safe way, it's called exportFunction. Now I think currently Cu.exportFunction(addEventListener, creteObjectIn(contentWindow),...) will not work cross process. Instead it would create a function object in the CPOW (or privileged junk) compartment, which is quite useless. I think it would make sense to prepare these API (exportFunction, createObjectIn, cloneInto) to handle CPOWs (so they would create object in the target process and then return a CPOW of the created object) Bobby, Bill, have you guys thought about this? What do you think?

I think it would only make sense to make these functions work with CPOWs if add-ons were using them a lot. The only uses I see, though, are from within the Jetpack SDK code itself or from content scripts of Jetpack add-ons. (There were one or two non-Jetpack add-ons I saw, but nothing worth mentioning.) If we see any other instances where exportFunction et al. would help then we can re-evaluate. But for now I think we should just fix pdf.js to use messaging. In my opinion it makes for a cleaner and more efficient interface.

I agree with billm.

Not sure if I can agree with that. Maybe I'm just confused so I write down my experiences and let me know what I'm missing. pdfjs/content/PdfjsChromeUtils.jsm lives in the chrome process, and pdfjs/content/PdfStreamConverter.jsm lives in the content process (but running with system principal as well.) and that is the reason we use the mm between them, and we need to use __exposedProps__ because on one side the CPOW is in the non-privileged compartment which is not helping here and we need to add __exposedProps__ to get anything working (http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm#300) Now unfortunately for us here __exposedProps__ is not working for callable props (which is a good thing in general). We need all this to listen to events across processes by the way (http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#750) so to do that instead I just sent the listener with mm to the chrome process (as CPOWs) and attached it there. Other solution is to just create listeners in the chrome process and then forward the events as CPOWS to PdfStreamConverter world. In both case we create a custom event in the chrome process so when we send it down to the content process it won't have access to it even with system principal (because of CPOW) unless we add a bunch of __exposedProps__ to it. I find this quite bad, and I do think our life would be simpler with the export helper API. So if I'm right that this is what's happening here, what is the imagined work flow to work with custom events between chrome and content processes when we have system principal js at both side?

In the meanwhile, probably I will just set up an event handler on chrome side and hope the events do not have any non-serializable properties and try to pass them with mm as serializables.

I don't think we should use CPOWs at all in the PDF code. The only data that we care about in the event is a string and some booleans: http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#777 It sounds like you're taking the right approach in comment 12. Content could ask the chrome process to set up an event listener for the three event types. When the event fires, chrome could message content and send the event details from the code I linked. It wouldn't need to send any CPOWs. Then content could create a custom event and dispatch it.

(In reply to Bill McCloskey (:billm) from comment #13) > I don't think we should use CPOWs at all in the PDF code. Yes, exactly. Very fundamentally, CPOWs are a crutch that are going away. If we run into issue with them in code we control, that's a great time to get rid of them.

Fwiw there's a bug filed already about getting rid of CPOWs in pdf.js e10s-mode: Bug 1071082

I think something like this. There are quite some stuff that I could have cleaned up better, but tried to focus on the necessary bits here only for now. What I couldn't figure out is how to do the "Only forward the events if they are for our dom window" check on the parent side nicely. I use the contentWindow CPOW for identity check there, and I would love to clean that part up and find a nicer way to do the same check. Who should I ask for help about it? I'm flagging you for feedback only since I really want to fix that part, and probably some polishing is still needed.

Comment on attachment 8565023 [details] [diff] [review] pdfjs rm cpows. v1 Review of attachment 8565023 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks pretty close. Just as a small nit, could you avoid the use of the aFoo parameter style in these files? It's not something they use right now, and I'd rather not introduce it. ::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm @@ +71,5 @@ > } > > function getFindBar(domWindow) { > if (PdfjsContentUtils.isRemote) { > + return null; Maybe just throw here? @@ +340,5 @@ > return false; > } > // ... and when the new find events code exists. > var findBar = getFindBar(this.domWindow); > + return PdfjsContentUtils.isRemote || findBar && ('updateControlState' in findBar); Nit: move the isRemote check to a separate if statement after the frameElement check. @@ +448,5 @@ > + .QueryInterface(Ci.nsIDocShell) > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIContentFrameMessageManager); > + > + winmm.sendSyncMessage('PDFJS:Parent:updateControlState', data); I'm pretty sure that all the messages here can be async. Is there any reason to make them sync? @@ +797,5 @@ > + forward.initCustomEvent(type, true, true, detail); > + // Due to restrictions with cpow use, we can't dispatch > + // dom events with an urgent message on the stack. So bounce > + // this off the main thread to make it async. > + Services.tm.mainThread.dispatch(function () { No need for this anymore. We can just dispatch the event right away. ::: browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm @@ +202,5 @@ > + }; > + > + // Only forward the events if they are for our dom window > + // XXX: this line is horrible please help to make this check saner! > + if (this._browser.ownerDocument.defaultView.gBrowser.selectedBrowser.contentWindowAsCPOW == this._events[e.type]) { Since this code is in a JSM that's supposed to manage all windows, I don't think we can have _events or _browser. Instead, I think we should have a Set of <browser> elements that are interested in findbar events. Then _addEventListener would add msg.target to the set (as well as register the listener). The code for handleEvent would get the chrome window from e.target.ownerDocument.defaultView. Then it would get the selected <browser> element from chromeWindow.gBrowser.selectedBrowser. If that <browser> is in the Set of elements that want findbar events, then we would send a message to that <browser> element. There should be no need to pass around the content window using this scheme. (Note: the content window will always be a top-level window because otherwise supportsIntegratedFind() would be false. So the <browser> element is an appropriate substitute for the content window.) @@ +203,5 @@ > + > + // Only forward the events if they are for our dom window > + // XXX: this line is horrible please help to make this check saner! > + if (this._browser.ownerDocument.defaultView.gBrowser.selectedBrowser.contentWindowAsCPOW == this._events[e.type]) { > + let mm = this._browser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.messageManager; This can be simplified to browser.messageManager.

(In reply to Bill McCloskey (:billm) from comment #17) > Thanks, this looks pretty close. Just as a small nit, could you avoid the > use of the aFoo parameter style in these files? It's not something they use > right now, and I'd rather not introduce it. I think PdfjsChromeUtils using it already so actually I should fix my 'e' parameter in handleEvent instead... > I'm pretty sure that all the messages here can be async. Is there any reason > to make them sync? I just wanted to be on the safe side first, I think there is no reason for it, so I want to do async for sure. > No need for this anymore. We can just dispatch the event right away. Yay! I forgot to ask you about this one, thanks. > Since this code is in a JSM that's supposed to manage all windows, I don't > think we can have _events or _browser. Instead, I think we should have a Set > of <browser> elements that are interested in findbar events. Then > _addEventListener would add msg.target to the set (as well as register the > listener). > > The code for handleEvent would get the chrome window from > e.target.ownerDocument.defaultView. Then it would get the selected <browser> > element from chromeWindow.gBrowser.selectedBrowser. If that <browser> is in > the Set of elements that want findbar events, then we would send a message > to that <browser> element. > > There should be no need to pass around the content window using this scheme. > (Note: the content window will always be a top-level window because > otherwise supportsIntegratedFind() would be false. So the <browser> element > is an appropriate substitute for the content window.) Thanks a lot for all this explanation, I was clearly confused about this part. I made the changes you suggested. I think that also means that we either want to have an array of event types for each browser that is registered, or instead (what I did) is to listen to all find events or none. So for the first browser we start listening to all find events and when the last browser is removed we stop listening. And we forward all find events when a browser is registered. There is only one problem I'm trying to wrap my head around currently. The browser I get from msg.target is not the same object I get as with chromeWindow.gBrowser.selectedBrowser. When I dump them I get [object XULElement] for the former and [object XULElement][Child 32383] for the later...

> When I dump them I get [object XULElement] for the former and > [object XULElement][Child 32383] for the later... Does that happen repeatedly? It sounds like an accident where the child process starts printing "[Child 32383] Foo bar" at the same time the parent process is printing "[object XULElement]". You can use Cu.getJSTestingFunctions().objectAddress(browser) to get the actual memory address of the <browser> element if you want to compare.

Thanks all the help! I hope it looks better now...

I'm attaching an interdiff as well, it might help the review since a lot of things have been changed...

Comment on attachment 8566149 [details] [diff] [review] pdfjs rm cpows. v2 Review of attachment 8566149 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Gabor. Looks nice. You'll need to ask Yuri Delendik for review since I'm not a pdf.js peer. The checkin procedure for pdf.js is also kind of complicated since the code is hosted on Github and occasionally merged over to mozilla-central. Yuri or Jim Mathies should be able to help with that. ::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm @@ +71,5 @@ > } > > function getFindBar(domWindow) { > if (PdfjsContentUtils.isRemote) { > + throw new Error('FindBar is not accessible from the content process.'); Double quotes for front-end code. @@ +786,3 @@ > }; > > +FindEventManager.prototype.receiveMessage = function(Msg) { Please call the param |msg|. ::: browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm @@ +193,5 @@ > + }, > + > + handleEvent: function(aEvent) { > + // We cannot just forward the message as a CPOW without setting up __exposedProps__ > + // on it. Instead, let's just create a streamable version of the event for performance Instead of "streamable" maybe say "structured cloneable". @@ +219,5 @@ > + 'findhighlightallchange', > + 'findcasesensitivitychange'], > + > + _addEventListener: function (aMsg) { > + this._browsers = this._browsers ? this._browsers : []; I think it would be cleaner to initialize this field in the init method. Also, it would be a lot cleaner to use a JavaScript Set. This code is Firefox-specific, so I don't think there's any reason not to. @@ +222,5 @@ > + _addEventListener: function (aMsg) { > + this._browsers = this._browsers ? this._browsers : []; > + let browser = aMsg.target; > + if (this._browsers.indexOf(browser) != -1) { > + throw new Error('FindEventManager was bound 2nd time without ubinding it first.'); unbinding @@ +229,5 @@ > + // Since this jsm is global, we need to store all the browsers > + // we have to forward the messages for. > + this._browsers.push(browser); > + > + if (this._browsers.length == 1) { I think we want to add the listeners unconditionally. Each browser has a separate findbar element (i.e., _findBarFromMessage returns different XUL elements for different browsers). @@ +248,5 @@ > + } > + > + this._browsers.array.splice(index, 1); > + > + if (this._browsers.length == 1) { Unconditional here as well.

Not sure if I'm doing this right, I don't use github too often... Let me know if I'm missing anything.

Yury, can we get a status update on this review? It's been a few weeks.

FYI, PDFJS's find works now at least, I don't know when this happened, but it implements its own findbar (not integrated) for e10s. It overrides the native findbar completely. So, you know... Either that or this. :)

since bug 1148192 closed, I'm assuming this should go with it.

Verified on Nightly 40.0a1 2015-04-06 under Mac OS X 10.9.5. Please see the results: - PDF integrated find bar is no longer present in this build - The searched words are found and highlighted - The number of matches is broken (e10s and non e10s), eg searching for a search term shows at first "3 of 3 matches", pressing Enter for a few times will still show "3 of 3 matches" instead of "1 of 3" / "2 of 3". Also, the number of matches is not consistent, searching may show "12 of 12 matches" and after pressing Enter/Next "23 of 23 matches" The last issue is not related to pdf's, I will search to see if it's already logged. I'm marking this bug as verified.