Closed Bug 1074983 Opened 10 years ago Closed 4 years ago

Fix potential races with CTP messages between browser-plugins and PluginContent

Categories

(Firefox :: General, defect, P3)

x86
All
defect

Tracking

()

RESOLVED INVALID
Tracking Status
e10s + ---

People

(Reporter: mconley, Assigned: mconley)

References

Details

Bug 899347 made click-to-play work for remote browsers by using message passing between the content process and the parent process. This, however, has opened us up to a few small race conditions. We've patched a few of the race conditions, but gfritzsche and I would like to find a more robust solution rather than plugging the races one by one. The problem is like this: Suppose I am on page A with some plugins. I travel to page B with some plugins, and then quickly browse back to page A. The message for the page B content can sometimes appear after we've returned to page A. We've fixed some of these cases by doing Principal comparison between the page that we're handling PluginContent messages from, and the currently selected browser (this was done in bug 1070053). We also deal with the fact that Data URIs inherit the Principals of whoever linked to them by analyzing the locations of the page that send the PluginContent message and the location of the currently selected browser (this was done in bug 1069075). This, however, does not account for the possibility of reloading the same page, and receiving messages from the content from before the reload _after_ the reload. It seems like there should be a way of eliminating this entire class of races. We should find that way.
From my understanding, we should have a way to invalidate in-flight messages or discard them later if they are from before navigating. I wonder if we don't have anything already that could let us identify that state.
tracking-e10s: --- → ?
Assignee: nobody → mconley
(In reply to Georg Fritzsche [:gfritzsche] from comment #1) > From my understanding, we should have a way to invalidate in-flight messages > or discard them later if they are from before navigating. > I wonder if we don't have anything already that could let us identify that > state. Do we share inner window IDs between the content and main processes?
I don't think so - but that's a decent idea. We could send those up instead of locations. I'm not even sure we need the Principals anymore with that plan. What do you think, gfritzsche?
Flags: needinfo?(georg.fritzsche)
Although, hm - right - unsure if the parent ever gets those inner window IDs to compare against. Let me think about this some more.
Flags: needinfo?(georg.fritzsche)
Just recording some conversation that billm and I had in IRC: Part of the problem is that the dismissel / removal of notifications happens in the parent when the parent notices that we change locations. What might be better is for PopupNotifications.jsm and the notificationbox XBL binding to inject frame scripts into the target browsers, and use messages to notice when locations change. That'd solve all of the above problems for CTP, because the messages are guaranteed to be received in order - even if we've browsed away and we get messages for the last page we were on, we're also going to get a message to remove those notifications right after. That might make testing CTP stuff a little tricky though - we'll need to wait for the PopupNotifications to be removed after browsing to a new page before proceeding.
Per last weeks talk, the most straight-forward (and most likely generally useful) way is probably to: * have a counter in the content * on every (real) navigation, increment it and message it to the parent * add the current counter state to (all or some) messages * now, thanks to defined message ordering, the parent code can compare its counter vs. the message counter * if the messages counter is less than the parents counter, we can discard the message in certain scenarios (like the CTP issues here) Mike, do you know who from e10s can confirm this is sane and not redundant to some other code we already have?
Flags: needinfo?(mconley)
(In reply to Georg Fritzsche [:gfritzsche] from comment #6) > Per last weeks talk, the most straight-forward (and most likely generally > useful) way is probably to: > * have a counter in the content > * on every (real) navigation, increment it and message it to the parent > * add the current counter state to (all or some) messages > * now, thanks to defined message ordering, the parent code can compare its > counter vs. the message counter > * if the messages counter is less than the parents counter, we can discard > the message in certain scenarios (like the CTP issues here) > > Mike, do you know who from e10s can confirm this is sane and not redundant > to some other code we already have? I guess that's me. :) It's not redundant - SessionStore uses a somewhat similar scheme, but it's not something general that we can just tag on. I don't think it'll be too bad to handroll this - I'll put something up in a few days.
Flags: needinfo?(mconley)
Mike, is this still a problem?
Flags: needinfo?(mconley)
Yes, I believe this is still a problem. (In reply to Mike Conley (:mconley) - Needinfo me! from comment #7) > I > don't think it'll be too bad to handroll this - I'll put something up in a > few days. *sigh*
Flags: needinfo?(mconley)
Priority: -- → P3

We're in the process of removing support for plugins (bug 1677160), so I think this bug is irrelevant now.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.